The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

Trunk
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

7 years ago
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

7 years ago
Blocks: 585538, 582858
(Assignee)

Comment 1

7 years ago
Created attachment 464240 [details] [diff] [review]
Patch v1
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

7 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++.
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)

Updated

7 years ago
Blocks: 585978
(Assignee)

Updated

7 years ago
No longer blocks: 582858
(Assignee)

Comment 6

7 years ago
That seems pretty sane to me.
(Assignee)

Comment 7

7 years ago
How about normalizing OS_TEST to i386 instead of modifying TARGET_CPU?
(Assignee)

Comment 8

7 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

7 years ago
Created attachment 464553 [details] [diff] [review]
Patch v2

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

7 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

7 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

7 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

7 years ago
Created attachment 464979 [details] [diff] [review]
Patch v3

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

7 years ago
Created attachment 464980 [details] [diff] [review]
Patch v3.1

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

7 years ago
Created attachment 465001 [details] [diff] [review]
Patch v4

Here's v4 with khuey's r+.
Attachment #464980 - Attachment is obsolete: true
(Assignee)

Comment 21

7 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

7 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
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

7 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.
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+
(Assignee)

Updated

6 years ago
Blocks: 616782
(Assignee)

Updated

6 years ago
Blocks: 585708
(Assignee)

Comment 28

6 years ago
http://hg.mozilla.org/mozilla-central/rev/b73633b8cb60
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Blocks: 625401
You need to log in before you can comment on or make changes to this bug.