Closed
Bug 585818
Opened 14 years ago
Closed 14 years ago
Define variable so Makefiles can easily tell if they're targeting either x86 or x64
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
Details
Attachments
(1 file, 4 obsolete files)
3.92 KB,
patch
|
benjamin
:
approval2.0+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #464240 -
Flags: review?(me)
So it looks like we do this already, at least on Windows http://mxr.mozilla.org/mozilla-central/source/configure.in#2475
Assignee | ||
Comment 3•14 years ago
|
||
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++.
Comment 5•14 years ago
|
||
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.
Assignee | ||
Comment 6•14 years ago
|
||
That seems pretty sane to me.
Assignee | ||
Comment 7•14 years ago
|
||
How about normalizing OS_TEST to i386 instead of modifying TARGET_CPU?
Assignee | ||
Comment 8•14 years ago
|
||
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.
Assignee | ||
Comment 10•14 years ago
|
||
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.
Assignee | ||
Comment 12•14 years ago
|
||
(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.
Assignee | ||
Comment 14•14 years ago
|
||
(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?
Assignee | ||
Comment 16•14 years ago
|
||
> (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...
Assignee | ||
Comment 17•14 years ago
|
||
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)
Assignee | ||
Comment 18•14 years ago
|
||
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+
Assignee | ||
Comment 20•14 years ago
|
||
Here's v4 with khuey's r+.
Attachment #464980 -
Attachment is obsolete: true
Assignee | ||
Comment 21•14 years ago
|
||
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?
Assignee | ||
Comment 22•14 years ago
|
||
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
Comment 23•14 years ago
|
||
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.
Assignee | ||
Comment 24•14 years ago
|
||
You're OK having busted VS2003 builds?
I am at least, we build with 2005 on tinderbox. Idk how many devs use 2003.
Comment 26•14 years ago
|
||
Yes, we can simply unsupport 2003 on trunk.
Comment 27•14 years ago
|
||
(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.
Updated•14 years ago
|
Attachment #465001 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 28•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•