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)
:
: Gregory Szorc [:gps] (away until 2017-03-20)
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 User image 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 User image Justin Lebar (not reading bugmail) 2010-08-09 17:34:01 PDT
Created attachment 464240 [details] [diff] [review]
Patch v1
Comment 2 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image 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 User image Justin Lebar (not reading bugmail) 2010-08-10 09:33:35 PDT
That seems pretty sane to me.
Comment 7 User image 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 User image 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-08-10 12:29:50 PDT
IIRC CPU_ARCH isn't exported from configure.  That isn't hard to fix though.
Comment 10 User image 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image 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 User image 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 User image 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 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image 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 User image 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 User image 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 User image 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 User image Justin Lebar (not reading bugmail) 2010-08-13 09:51:25 PDT
You're OK having busted VS2003 builds?
Comment 25 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 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 User image Ted Mielczarek [:ted.mielczarek] 2010-08-13 09:58:10 PDT
Yes, we can simply unsupport 2003 on trunk.
Comment 27 User image 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 User image 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.