Last Comment Bug 585818 - Define variable so Makefiles can easily tell if they're targeting either x86 or x64
: Define variable so Makefiles can easily tell if they're targeting either x86 ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on:
Blocks: 585538 585708 585978 616782 625401
  Show dependency treegraph
 
Reported: 2010-08-09 17:23 PDT by Justin Lebar (not reading bugmail)
Modified: 2011-01-13 09:14 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.51 KB, patch)
2010-08-09 17:34 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v2 (2.34 KB, patch)
2010-08-10 13:45 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v3 (2.34 KB, patch)
2010-08-11 15:37 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v3.1 (1.96 KB, patch)
2010-08-11 15:38 PDT, Justin Lebar (not reading bugmail)
khuey: review+
Details | Diff | Splinter Review
Patch v4 (3.92 KB, patch)
2010-08-11 16:21 PDT, Justin Lebar (not reading bugmail)
benjamin: approval2.0+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2010-08-09 17:23:57 PDT
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) 2010-08-09 17:34:01 PDT
Created attachment 464240 [details] [diff] [review]
Patch v1
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-08-09 18:32:38 PDT
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) 2010-08-09 18:44:40 PDT
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 [:khuey] (khuey@mozilla.com) 2010-08-09 18:46:59 PDT
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 Ted Mielczarek [:ted.mielczarek] 2010-08-10 05:28:15 PDT
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) 2010-08-10 09:33:35 PDT
That seems pretty sane to me.
Comment 7 Justin Lebar (not reading bugmail) 2010-08-10 12:25:31 PDT
How about normalizing OS_TEST to i386 instead of modifying TARGET_CPU?
Comment 8 Justin Lebar (not reading bugmail) 2010-08-10 12:27:37 PDT
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 [:khuey] (khuey@mozilla.com) 2010-08-10 12:29:50 PDT
IIRC CPU_ARCH isn't exported from configure.  That isn't hard to fix though.
Comment 10 Justin Lebar (not reading bugmail) 2010-08-10 13:45:51 PDT
Created attachment 464553 [details] [diff] [review]
Patch v2

Expose CPU_ARCH to Makefiles.
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-08-10 22:58:52 PDT
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) 2010-08-10 23:13:38 PDT
(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 [:khuey] (khuey@mozilla.com) 2010-08-10 23:30:07 PDT
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) 2010-08-10 23:37:13 PDT
(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 [:khuey] (khuey@mozilla.com) 2010-08-10 23:47:12 PDT
(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) 2010-08-11 09:27:59 PDT
> (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) 2010-08-11 15:37:08 PDT
Created attachment 464979 [details] [diff] [review]
Patch v3

Here's a patch which exposes CPU_ARCH and INTEL_ARCHITECTURE, as we discussed.
Comment 18 Justin Lebar (not reading bugmail) 2010-08-11 15:38:00 PDT
Created attachment 464980 [details] [diff] [review]
Patch v3.1

Oops; wrong version.
Comment 19 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-08-11 15:44:14 PDT
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) 2010-08-11 16:21:26 PDT
Created attachment 465001 [details] [diff] [review]
Patch v4

Here's v4 with khuey's r+.
Comment 21 Justin Lebar (not reading bugmail) 2010-08-12 10:09:34 PDT
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) 2010-08-13 09:37:44 PDT
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 Ted Mielczarek [:ted.mielczarek] 2010-08-13 09:49:33 PDT
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) 2010-08-13 09:51:25 PDT
You're OK having busted VS2003 builds?
Comment 25 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-08-13 09:56:10 PDT
I am at least, we build with 2005 on tinderbox.  Idk how many devs use 2003.
Comment 26 Ted Mielczarek [:ted.mielczarek] 2010-08-13 09:58:10 PDT
Yes, we can simply unsupport 2003 on trunk.
Comment 27 neil@parkwaycc.co.uk 2010-08-13 12:03:09 PDT
(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) 2011-01-12 22:18:03 PST
http://hg.mozilla.org/mozilla-central/rev/b73633b8cb60

Note You need to log in before you can comment on or make changes to this bug.