585818 - Define variable so Makefiles can easily tell if they're targeting either x86 or x64

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

It's hard to tell in a makefile if you're targeting x86.  configure.in tests that OS_TEST matches i?86, but I'm not aware of a way to do such wildcarding in Make.

Patch forthcoming.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So it looks like we do this already, at least on Windows http://mxr.mozilla.org/mozilla-central/source/configure.in#2475

Comment 3

[Justin Lebar (not reading bugmail)]

I'd be happy to move that code out of the windows-only section.  I'd probably also want to set the variables to 1 as opposed to just defining them, since that lets you do

  ifneq (,$(_X86_)$(_AMD64_))

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

You'll also want to AC_SUBST and put these in autoconf.mk.in so that they're available in makefiles.  AC_DEFINE only gets them available to c++.

Comment 5

[(not currently active) Ted Mielczarek]

Could we normalize TARGET_CPU to be "i386" when targeting x86 in general? I can't imagine that we actually have any Makefile code that wants to discriminate between i386 and i586.

Comment 6

[Justin Lebar (not reading bugmail)]

That seems pretty sane to me.

Comment 7

[Justin Lebar (not reading bugmail)]

How about normalizing OS_TEST to i386 instead of modifying TARGET_CPU?

Comment 8

[Justin Lebar (not reading bugmail)]

Oh...there's apparently a CPU_ARCH variable which is set to x86 or x86_64, just like we want.

Man, there's a lot of magic in here.

I'll try it and see if it works.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

IIRC CPU_ARCH isn't exported from configure.  That isn't hard to fix though.

Comment 10

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Expose CPU_ARCH to Makefiles.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

What is the goal of INTEL_ARCHITECTURE?  If you actually do need it you should be AC_SUBSTing it too.

Comment 12

[Justin Lebar (not reading bugmail)]

(In reply to comment #11)
> What is the goal of INTEL_ARCHITECTURE?  If you actually do need it you should
> be AC_SUBSTing it too.

I totally thought I'd deleted that.

On second thought, however, it *would* be nice to have a single "X86 or X86_64" variable; otherwise, you have to use |filter| on CPU_ARCH, which is just a pain.  The whole point of this is to lower the activation energy required to use SIMD, after all.

I'm not at all attached to the name, though.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, the name sucks.  Do we support/plan to support any non-x86/ARM NEON SIMD instruction sets?  If not, lets call it SIMD_[INTEL|ARM] or something similar.

Comment 14

[Justin Lebar (not reading bugmail)]

(In reply to comment #13)
> Yeah, the name sucks.  Do we support/plan to support any non-x86/ARM NEON SIMD
> instruction sets?  If not, lets call it SIMD_[INTEL|ARM] or something similar.

Well, that's not quite right, since the fact that we're targeting x86/x64 or ARM doesn't mean that the target supports SSE/NEON.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #14)
> (In reply to comment #13)
> > Yeah, the name sucks.  Do we support/plan to support any non-x86/ARM NEON SIMD
> > instruction sets?  If not, lets call it SIMD_[INTEL|ARM] or something similar.
> 
> Well, that's not quite right, since the fact that we're targeting x86/x64 or
> ARM doesn't mean that the target supports SSE/NEON.

You're talking about an ARM/x86 compiler that doesn't support the relevant SIMD intrinsics, right?  If that's the case, how is INTEL_ARCHITECTURE going to help?

Comment 16

[Justin Lebar (not reading bugmail)]

> (In reply to comment #15)
> You're talking about an ARM/x86 compiler that doesn't support the relevant SIMD
> intrinsics, right?  If that's the case, how is INTEL_ARCHITECTURE going to
> help?

I mean that SIMD_ARM is a misleading name in that it sounds like we can assume that the target supports NEON, when in fact it means that the *compiler* supports NEON.

Perhaps the right thing is to define a variable which is 1 if the compiler supports a specific set of SIMD extensions for the target at hand.  For instance, my understanding is that VS2003 doesn't support past SSE2, so we would define COMPILER_SUPPORTS_MMX, COMPILER_SUPPORTS_SSE, and COMPILER_SUPPORTS_SSE2 on that platform, but not COMPILER_SUPPORTS_SSE3.

Of course, that's a lot more work, because we have to fill in that compatibility matrix...

Comment 17

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

Here's a patch which exposes CPU_ARCH and INTEL_ARCHITECTURE, as we discussed.

Comment 18

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3.1

Oops; wrong version.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 464980 [details] [diff] [review]
Patch v3.1

WFM, but this all needs to be duplicated in js/src.

Comment 20

[Justin Lebar (not reading bugmail)]

Attachment: Patch v4

Here's v4 with khuey's r+.

Comment 21

[Justin Lebar (not reading bugmail)]

Comment on attachment 465001 [details] [diff] [review]
Patch v4

Requesting approval 2.0.  This is a very low-risk change -- we're just exposing a few new variables -- and it will make the build much cleaner where we use x86 intrinsics (see e.g. bug 585538).

Comment 22

[Justin Lebar (not reading bugmail)]

Hm.  We may need to go the way of defining separate COMPILER_SUPPORTS_X variables.  Bug 506430 needed a bustage fix for VS older than v1400 (2005?).  I'm going to ask Neil about it after lunch, and will report back here.

Comment 23

[(not currently active) Ted Mielczarek]

1400 is 2005, yes. I don't really care about supporting anything older than that if it's a pain. VC 2003 is pretty old at this point.

Comment 24

[Justin Lebar (not reading bugmail)]

You're OK having busted VS2003 builds?

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I am at least, we build with 2005 on tinderbox.  Idk how many devs use 2003.

Comment 26

[(not currently active) Ted Mielczarek]

Yes, we can simply unsupport 2003 on trunk.

Comment 27

[[e-mail]]

(In reply to comment #22)
> Hm.  We may need to go the way of defining separate COMPILER_SUPPORTS_X
> variables.  Bug 506430 needed a bustage fix for VS older than v1400 (2005?). 
> I'm going to ask Neil about it after lunch, and will report back here.
VC7.1 doesn't have __sse2_available. I don't know what replaced it on trunk.

Comment 28

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/b73633b8cb60