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

RESOLVED FIXED in mozilla15

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gaston, Assigned: gaston)

Tracking

Trunk
mozilla15
Other
OpenBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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)

Updated

5 years ago
Depends on: 755869
(Assignee)

Comment 1

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

Comment 2

5 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

5 years ago
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
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
(Assignee)

Updated

5 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

5 years ago
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)
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+
(Assignee)

Comment 9

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

5 years ago
Created attachment 628745 [details] [diff] [review]
v3

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

Updated

5 years ago
Keywords: checkin-needed

Comment 12

5 years ago
FYI this also broke clang lto build: http://llvm.org/bugs/show_bug.cgi?id=13000
(Assignee)

Comment 13

5 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

5 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

5 years ago
Attachment #628745 - Flags: checkin?(dzbarsky)
Comment on attachment 628745 [details] [diff] [review]
v3

https://hg.mozilla.org/integration/mozilla-inbound/rev/fca2f3541a72
Attachment #628745 - Flags: checkin?(dzbarsky) → checkin+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fca2f3541a72
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Assignee)

Updated

5 years ago
Depends on: 786995
You need to log in before you can comment on or make changes to this bug.