Closed Bug 464127 Opened 16 years ago Closed 16 years ago

Don't use -Os with Intel C/C++ compilers

Categories

(Core :: JavaScript Engine, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jimb, Assigned: jimb)

Details

Attachments

(1 file, 2 obsolete files)

Intel recommends against the use of -Os, and using it seems to produce incorrect code in many recent versions of Intel's compilers.

js/src/Makefile.in tries to use -Os only with GCC, but it only tests INTEL_CC, ignoring INTEL_CXX.
Intel recommends against the use of -Os, and using it seems to produce
incorrect code in many recent versions of Intel's compilers.

js/src/Makefile.in tries to use -Os only with GCC, but it only tests INTEL_CC,
ignoring INTEL_CXX.

(The try server likes this patch.)
Attachment #347484 - Flags: review?(ted.mielczarek)
Attachment #347484 - Flags: review?(ted.mielczarek) → review-
Comment on attachment 347484 [details] [diff] [review]
Bug 464127 -  Don't use -Os with Intel C/C++ compilers

 *-*linux*)
-    if test "$GNU_CC"; then
+    # Note: both GNU_CC and INTEL_CC are set when using Intel's C compiler.
+    # Similarly for GNU_CXX and INTEL_CXX.
+    if test "$INTEL_CC" || test "$INTEL_CXX"; then

This is in the Linux block. I didn't think anyone was using ICC on Linux.

-ifdef GNU_CC
-ifdef INTEL_CC
+# The $(value X) construct lets us test X's unexpanded value, just as
+# 'ifdef' does.
+ifneq (,$(value GNU_CC)$(value GNU_CXX))
+ifneq (,$(value INTEL_CC)$(value INTEL_CXX))

Since SpiderMonkey is all-C++, you should only have to test INTEL_CXX now.
(In reply to comment #2)
> (From update of attachment 347484 [details] [diff] [review])
>  *-*linux*)
> -    if test "$GNU_CC"; then
> +    # Note: both GNU_CC and INTEL_CC are set when using Intel's C compiler.
> +    # Similarly for GNU_CXX and INTEL_CXX.
> +    if test "$INTEL_CC" || test "$INTEL_CXX"; then
> 
> This is in the Linux block. I didn't think anyone was using ICC on Linux.

ICC does support Linux; shouldn't we try to make it work?

> -ifdef GNU_CC
> -ifdef INTEL_CC
> +# The $(value X) construct lets us test X's unexpanded value, just as
> +# 'ifdef' does.
> +ifneq (,$(value GNU_CC)$(value GNU_CXX))
> +ifneq (,$(value INTEL_CC)$(value INTEL_CXX))
> 
> Since SpiderMonkey is all-C++, you should only have to test INTEL_CXX now.

Okay, sure.
Intel recommends against the use of -Os, and using it seems to produce
incorrect code in many recent versions of Intel's compilers.

js/src/Makefile.in tries to use -Os only with G++, but it tests
INTEL_CC, not INTEL_CXX --- even though almost all the sources are
C++.  Forbid the use of mixed Intel/GNU compilers, and check
INTEL_CXX.
Attachment #347484 - Attachment is obsolete: true
Attachment #347628 - Flags: review?(ted.mielczarek)
Comment on attachment 347628 [details] [diff] [review]
Bug 464127 -  Don't use -Os with Intel C/C++ compilers

+dnl You can't mix and match C and C++ compilers.  Certainly the
+dnl Mozilla Makefiles don't allow us to establish different
+dnl MOZ_OPTIMIZE_FLAGS and MOZ_DEBUG_FLAGS for the C and C++
+dnl compilers.

I don't know that I agree with this. For example, we may not be able to build NSPR with icc, but we could build js/src with icpc, so we might wind up using gcc and icpc together.

Regardless, does SpiderMonkey have any straight C files left that matter?
Our current build system means that mixing compilers requires us to use least-common-denominator compiler flags.  Our tree still has significant stretches of C code, so this doesn't seem useful to me.

That aside, js/src is, in fact, almost entirely C++ (and could be made completely so without too much effort), so I'll go along with you that C++ is all we care about here.
Intel recommends against the use of -Os, and using it seems to produce
incorrect code in many recent versions of Intel's compilers.

js/src/Makefile.in tries to use -Os only with G++, but it tests
INTEL_CC, not INTEL_CXX --- even though almost all the sources are
C++.  Check INTEL_CXX instead.
Attachment #347628 - Attachment is obsolete: true
Attachment #347798 - Flags: review?(ted.mielczarek)
Attachment #347628 - Flags: review?(ted.mielczarek)
Assignee: general → jim
Status: NEW → ASSIGNED
Attachment #347798 - Flags: review?(ted.mielczarek) → review+
Landed in tracemonkey hg as changeset 19028cd2bad2.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.