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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
9 years ago
Last year

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

Trunk
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Blocks: 585538, 582858
Posted patch Patch v1 (obsolete) — Splinter Review
Attachment #464240 - Flags: review?(me)
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_))
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++.
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.
Blocks: 585978
No longer blocks: 582858
That seems pretty sane to me.
How about normalizing OS_TEST to i386 instead of modifying TARGET_CPU?
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.
IIRC CPU_ARCH isn't exported from configure.  That isn't hard to fix though.
Posted patch Patch v2 (obsolete) — Splinter Review
Expose CPU_ARCH to Makefiles.
Attachment #464240 - Attachment is obsolete: true
Attachment #464553 - Flags: review?(me)
Attachment #464240 - Flags: review?(me)
What is the goal of INTEL_ARCHITECTURE?  If you actually do need it you should be AC_SUBSTing it too.
(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.
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.
(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.
(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?
> (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...
Posted patch Patch v3 (obsolete) — Splinter Review
Here's a patch which exposes CPU_ARCH and INTEL_ARCHITECTURE, as we discussed.
Assignee: nobody → justin.lebar+bug
Attachment #464553 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #464979 - Flags: review?(me)
Attachment #464553 - Flags: review?(me)
Posted patch Patch v3.1 (obsolete) — Splinter Review
Oops; wrong version.
Attachment #464979 - Attachment is obsolete: true
Attachment #464980 - Flags: review?(me)
Attachment #464979 - Flags: review?(me)
Comment on attachment 464980 [details] [diff] [review]
Patch v3.1

WFM, but this all needs to be duplicated in js/src.
Attachment #464980 - Flags: review?(me) → review+
Posted patch Patch v4Splinter Review
Here's v4 with khuey's r+.
Attachment #464980 - Attachment is obsolete: true
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).
Attachment #465001 - Flags: approval2.0?
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.
Status: ASSIGNED → NEW
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.
You're OK having busted VS2003 builds?
I am at least, we build with 2005 on tinderbox.  Idk how many devs use 2003.
Yes, we can simply unsupport 2003 on trunk.
(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.
Attachment #465001 - Flags: approval2.0? → approval2.0+
Blocks: 616782
Blocks: 585708
http://hg.mozilla.org/mozilla-central/rev/b73633b8cb60
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 625401
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.