Last Comment Bug 759683 - cc1plus: error: unrecognized command line option "-mssse3" after bug 755869
: cc1plus: error: unrecognized command line option "-mssse3" after bug 755869
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: Other OpenBSD
: -- normal (vote)
: mozilla15
Assigned To: Landry Breuil (:gaston)
:
: Gregory Szorc [:gps]
Mentors:
Depends on: 755869 786995
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-30 01:25 PDT by Landry Breuil (:gaston)
Modified: 2012-08-30 03:47 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1, add compile time mssse3 check + make runtime check conditional to it (4.00 KB, patch)
2012-05-30 03:45 PDT, Landry Breuil (:gaston)
mh+mozilla: review-
Details | Diff | Splinter Review
v2 (6.38 KB, patch)
2012-05-30 13:45 PDT, Landry Breuil (:gaston)
mh+mozilla: review+
Details | Diff | Splinter Review
v3 (5.25 KB, patch)
2012-05-31 07:55 PDT, Landry Breuil (:gaston)
dzbarsky: checkin+
Details | Diff | Splinter Review

Description Landry Breuil (:gaston) 2012-05-30 01:25:42 PDT
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) 2012-05-30 01:40:40 PDT
And it seems it will probably affect macos too (which still uses 4.2 ?) when skia is enabled there.
Comment 2 Landry Breuil (:gaston) 2012-05-30 02:03:08 PDT
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) 2012-05-30 03:45:16 PDT
Created attachment 628300 [details] [diff] [review]
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] 2012-05-30 04:04:05 PDT
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] 2012-05-30 04:06:00 PDT
Oh, and you need to add the skia patch and update gfx/skia/update.sh
Comment 6 George Wright (:gw280) (:gwright) 2012-05-30 13:44:38 PDT
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) 2012-05-30 13:45:07 PDT
Created attachment 628456 [details] [diff] [review]
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] 2012-05-31 05:17:06 PDT
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) 2012-05-31 07:38:32 PDT
(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] 2012-05-31 07:42:05 PDT
(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) 2012-05-31 07:55:27 PDT
Created attachment 628745 [details] [diff] [review]
v3

Address comments and rebase after comment 6/patches move. Carrying r+ forward.
Comment 12 Octoploid 2012-06-01 00:48:05 PDT
FYI this also broke clang lto build: http://llvm.org/bugs/show_bug.cgi?id=13000
Comment 13 Landry Breuil (:gaston) 2012-06-01 01:12:26 PDT
(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 2012-06-01 01:37:14 PDT
(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.

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