Closed Bug 759683 Opened 8 years ago Closed 8 years ago

cc1plus: error: unrecognized command line option "-mssse3" after bug 755869

Categories

(Firefox Build System :: General, defect)

Other
OpenBSD
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla15

People

(Reporter: gaston, Assigned: gaston)

References

Details

Attachments

(2 files, 1 obsolete file)

Skia update brought gfx/skia/src/opts/SkBitmapProcState_opts_SSSE3.cpp in and the build bits in https://hg.mozilla.org/mozilla-central/rev/477b8a5a7169 to compile it. Unfortunately -mssse3 was introduced in Gcc 4.3 (see http://gcc.gnu.org/gcc-4.3/changes.html) so this breaks the build on Gcc 4.2.1...
I'll see how to gracefully make it conditional to the gcc version, if possible.
Depends on: 755869
And it seems it will probably affect macos too (which still uses 4.2 ?) when skia is enabled there.
Hm, disregard last comment, from https://tbpl.mozilla.org/php/getParsedLog.php?id=12155253&tree=Firefox&full=1 it seems macos built that file fine with -mssse3, while it uses gcc 4.2.. puzzling..
First version of the patch (a slightly different version was sent to try : https://tbpl.mozilla.org/?tree=Try&rev=ec0ee5d599d6 - i've checked linux/macos logs and the SSSE3 file was built)

- renamed the define to HAVE_COMPILER_FLAGS_MSSSE3 to avoid being GCC specific
- i've realized that if the SSSE3 parts wasnt built, then it should be called in opts_check_SSE2.cpp, hence the hasSSSE3 returning false. That part wasnt testbuilt, i'm not sure HAVE_COMPILER_FLAGS_MSSSE3 is propagated to #defines (doesn't seem to be in mozilla-config.h), what more would be needed ?
- of course the last chunk should also be made a local patch, sent upstream
Assignee: nobody → landry
Attachment #628300 - Flags: review?(mh+mozilla)
Comment on attachment 628300 [details] [diff] [review]
v1, add compile time mssse3 check + make runtime check conditional to it

Review of attachment 628300 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/autoconf.mk.in
@@ +657,5 @@
>  
>  HAVE_ARM_SIMD = @HAVE_ARM_SIMD@
>  HAVE_ARM_NEON = @HAVE_ARM_NEON@
>  HAVE_GCC_ALIGN_ARG_POINTER = @HAVE_GCC_ALIGN_ARG_POINTER@
> +HAVE_COMPILER_FLAGS_MSSSE3 = @HAVE_COMPILER_FLAGS_MSSSE3@

nit: FLAG, singular

::: gfx/skia/src/opts/opts_check_SSE2.cpp
@@ +77,5 @@
>      return (cpu_info[3] & (1<<26)) != 0;
>  }
>  #endif
>  
> +#if defined (HAVE_COMPILER_FLAGS_MSSSE3)

This should probably be treated like the SK_BUILD_FOR_ANDROID case in SkBitmapProcState::platformProcs in the same file. Also note that this is a Skia file, and that it shouldn't depend on Mozilla variables. I'd go for something like SK_BUILD_SSSE3 (replacing SK_BUILD_FOR_ANDROID in SkBitmapProcState::platformProcs altogether), and add it to DEFINES in gfx/skia/Makefile.in, when HAVE_COMPILER_FLAGS_MSSSE3 is defined. BTW, since you don't define HAVE_COMPILER_FLAGS_MSSSE3 anywhere for compilation, this patch actually effectively disables SSSE3 support.
Obviously, when this is upstreamed, it would need some gyp rules to define SK_BUILD_SSSE3 appropriately.
Attachment #628300 - Flags: review?(mh+mozilla) → review-
Oh, and you need to add the skia patch and update gfx/skia/update.sh
If you do, I just landed a patch to reorganise the patches applied to Skia. See the final patch on bug 755869
Attached patch v2Splinter Review
That new patch should address all the comments. Builds for me here with gcc 4.2.1, and seems to cause no harm on tier1 platforms, see https://tbpl.mozilla.org/?tree=Try&rev=1d6b08afb83c (i've checked that the SSSE3 file was built on mac/linux and not on android)
Attachment #628300 - Attachment is obsolete: true
Attachment #628456 - Flags: review?(mh+mozilla)
Comment on attachment 628456 [details] [diff] [review]
v2

Review of attachment 628456 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/skia/src/opts/opts_check_SSE2.cpp
@@ +77,5 @@
>      return (cpu_info[3] & (1<<26)) != 0;
>  }
>  #endif
>  
> +#if defined (SK_BUILD_SSSE3)

You shouldn't need this.

@@ +107,1 @@
>          // Disable SSSE3 optimization for Android x86

Please change the comment.

And see comment 6.
Attachment #628456 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #8)
> Comment on attachment 628456 [details] [diff] [review]
> v2
> 
> Review of attachment 628456 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/skia/src/opts/opts_check_SSE2.cpp
> @@ +77,5 @@
> >      return (cpu_info[3] & (1<<26)) != 0;
> >  }
> >  #endif
> >  
> > +#if defined (SK_BUILD_SSSE3)
> 
> You shouldn't need this.

You mean the whole #if block adding the hasSSSE3() block ?
(In reply to Landry Breuil (:gaston) from comment #9)
> You mean the whole #if block adding the hasSSSE3() block ?

I mean you shouldn't need to change hasSSSE3().
Attached patch v3Splinter Review
Address comments and rebase after comment 6/patches move. Carrying r+ forward.
Keywords: checkin-needed
FYI this also broke clang lto build: http://llvm.org/bugs/show_bug.cgi?id=13000
(In reply to Octoploid from comment #12)
> FYI this also broke clang lto build:
> http://llvm.org/bugs/show_bug.cgi?id=13000

Yeah but does the proposed fix fixes it ? I doubt so..
(In reply to Landry Breuil (:gaston) from comment #13)
> (In reply to Octoploid from comment #12)
> > FYI this also broke clang lto build:
> > http://llvm.org/bugs/show_bug.cgi?id=13000
> 
> Yeah but does the proposed fix fixes it ? I doubt so..

No, it doesn't.
Attachment #628745 - Flags: checkin?(dzbarsky)
https://hg.mozilla.org/mozilla-central/rev/fca2f3541a72
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Depends on: 786995
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.