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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Landry Breuil (:gaston)]

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.

Comment 1

[Landry Breuil (:gaston)]

And it seems it will probably affect macos too (which still uses 4.2 ?) when skia is enabled there.

Comment 2

[Landry Breuil (:gaston)]

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

Comment 3

[Landry Breuil (:gaston)]

Attachment: v1, add compile time mssse3 check + make runtime check conditional to it

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

Comment 4

[Mike Hommey [:glandium]]

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.

Comment 5

[Mike Hommey [:glandium]]

Oh, and you need to add the skia patch and update gfx/skia/update.sh

Comment 6

[George Wright (:gw280) (needinfo me!)]

If you do, I just landed a patch to reorganise the patches applied to Skia. See the final patch on bug 755869

Comment 7

[Landry Breuil (:gaston)]

Attachment: v2

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)

Comment 8

[Mike Hommey [:glandium]]

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.

Comment 9

[Landry Breuil (:gaston)]

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

Comment 10

[Mike Hommey [:glandium]]

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

Comment 11

[Landry Breuil (:gaston)]

Attachment: v3

Address comments and rebase after comment 6/patches move. Carrying r+ forward.

Comment 12

[Octoploid]

FYI this also broke clang lto build: http://llvm.org/bugs/show_bug.cgi?id=13000

Comment 13

[Landry Breuil (:gaston)]

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

Comment 14

[Octoploid]

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

Comment 15

[David Zbarsky (:dzbarsky)]

Comment on attachment 628745 [details] [diff] [review]
v3

https://hg.mozilla.org/integration/mozilla-inbound/rev/fca2f3541a72

Comment 16

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/fca2f3541a72