Closed
Bug 759683
Opened 12 years ago
Closed 12 years ago
cc1plus: error: unrecognized command line option "-mssse3" after bug 755869
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla15
People
(Reporter: gaston, Assigned: gaston)
References
Details
Attachments
(2 files, 1 obsolete file)
6.38 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
5.25 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
And it seems it will probably affect macos too (which still uses 4.2 ?) when skia is enabled there.
Assignee | ||
Comment 2•12 years ago
|
||
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..
Assignee | ||
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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-
Comment 5•12 years ago
|
||
Oh, and you need to add the skia patch and update gfx/skia/update.sh
Assignee | ||
Updated•12 years ago
|
Comment 6•12 years ago
|
||
If you do, I just landed a patch to reorganise the patches applied to Skia. See the final patch on bug 755869
Assignee | ||
Comment 7•12 years ago
|
||
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 8•12 years ago
|
||
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+
Assignee | ||
Comment 9•12 years ago
|
||
(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•12 years ago
|
||
(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().
Assignee | ||
Comment 11•12 years ago
|
||
Address comments and rebase after comment 6/patches move. Carrying r+ forward.
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 12•12 years ago
|
||
FYI this also broke clang lto build: http://llvm.org/bugs/show_bug.cgi?id=13000
Assignee | ||
Comment 13•12 years ago
|
||
(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•12 years ago
|
||
(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.
Assignee | ||
Updated•12 years ago
|
Attachment #628745 -
Flags: checkin?(dzbarsky)
Comment 15•12 years ago
|
||
Comment on attachment 628745 [details] [diff] [review] v3 https://hg.mozilla.org/integration/mozilla-inbound/rev/fca2f3541a72
Attachment #628745 -
Flags: checkin?(dzbarsky) → checkin+
Updated•12 years ago
|
Keywords: checkin-needed
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fca2f3541a72
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•