Open Bug 1028827 Opened 10 years ago Updated 2 years ago

Build uses -mssse3 and fails on OS where autoconf test for -mssse3 fails

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Version:
30 Branch
Platform:
x86
OpenBSD
Type:
defect

Tracking

(firefox36 affected, firefox37 affected, firefox38 affected, firefox39 affected)

Tracking Flags:
Tracking Status
firefox36 --- affected
firefox37 --- affected
firefox38 --- affected
firefox39 --- affected

People

(Reporter: stu-mozilla-persona, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

fx-i386.log
5.52 KB, text/x-log
Details
skia-moz.build.diff
1.90 KB, patch
Details | Diff | Splinter Review
simpler generate_mozbuild fix
8.42 KB, patch
Details | Diff | Splinter Review
bug-1028827-sse3-sse4_v2.patch
8.25 KB, patch
Details | Diff | Splinter Review
Attached file fx-i386.log — 
User Agent: Mozilla/5.0 (X11; OpenBSD amd64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.155 Safari/537.36

Steps to reproduce:

Attempt to build Firefox 30.0 from ports on OpenBSD/i386.


Actual results:

autoconf checks decided that ssse3 was not supported:

checking if toolchain supports -mssse3 option... no
checking if toolchain supports -msse4.1 option... no
checking for x86 AVX2 asm support in compiler... no

media/libvpx/vp8/common/x86/variance_ssse3.c was built with -mssse3 (compilation succeeded).

gfx/skia/trunk/src/opts/SkBitmapProcState_opts_SSSE3.cpp was attempted with -mssse3 (compilation failed; OpenBSD binutils is too old for pmaddubsw).

Partial log excerpt attached, full log at http://junkpile.org/mozilla-firefox-i386-20140623.log

Should these SSSE3 files be built at all if ssse3 is detected as not supported?

Builds of previous versions (e.g. 29.0.1) successful.


Expected results:

Build succeeded.
Hardware: x86_64 → x86
Just ftp, there were numerous bugs in the past about ssse3/sse2/ssse41/etc, at least :

https://bugzilla.mozilla.org/show_bug.cgi?id=759683
https://bugzilla.mozilla.org/show_bug.cgi?id=1023028
https://bugzilla.mozilla.org/show_bug.cgi?id=880976
https://bugzilla.mozilla.org/show_bug.cgi?id=878446
https://bugzilla.mozilla.org/show_bug.cgi?id=786995

probably missing some #if around in moz.build ... looking at it, seems like a comeback of https://bugzilla.mozilla.org/show_bug.cgi?id=759683. Maybe a skia update removed some patches related to those.
This was broken in the skia update http://hg.mozilla.org/mozilla-central/rev/f6c2efda0788
I'm testing with patch I'm about to attach as skia-moz.build.diff (just handles skia, ignoring libvpx for now), I'll update the ticket when done,
Attached patch skia-moz.build.diff (obsolete) — — Splinter Review 
Blocks: 910754

    
    

    
  
Probably related to 1023732 too...
See Also:  → 1023732

    
    

    
  


  
update; add missing piece

        Attachment #8444294 -
        Attachment is obsolete: true

    
    

    
  
My diff isn't sufficient;

/usr/bin/ld: Warning: gc-sections option ignored
../../gfx/skia/opts_check_SSE2.o(.text._ZN17SkBitmapProcState13platformProcsEv+0x13a): In function `SkBitmapProcState::platformProcs()':
: undefined reference to `S32_opaque_D32_filter_DXDY_SSSE3(SkBitmapProcState const&, unsigned int const*, int, unsigned int*)'
../../gfx/skia/opts_check_SSE2.o(.text._ZN17SkBitmapProcState13platformProcsEv+0x14a): In function `SkBitmapProcState::platformProcs()':
: undefined reference to `S32_opaque_D32_filter_DX_SSSE3(SkBitmapProcState const&, unsigned int const*, int, unsigned int*)'
../../gfx/skia/opts_check_SSE2.o(.text._ZN17SkBitmapProcState13platformProcsEv+0x16a): In function `SkBitmapProcState::platformProcs()':
: undefined reference to `S32_alpha_D32_filter_DX_SSSE3(SkBitmapProcState const&, unsigned int const*, int, unsigned int*)'
../../gfx/skia/opts_check_SSE2.o(.text._ZN17SkBitmapProcState13platformProcsEv+0x17a): In function `SkBitmapProcState::platformProcs()':
: undefined reference to `S32_alpha_D32_filter_DXDY_SSSE3(SkBitmapProcState const&, unsigned int const*, int, unsigned int*)'
/usr/bin/ld: libxul.so.48.0: hidden symbol `_Z31S32_alpha_D32_filter_DXDY_SSSE3RK17SkBitmapProcStatePKjiPj' isn't defined
collect2: error: ld returned 1 exit status
/usr/obj/ports/firefox-30.0/mozilla-release/config/rules.mk:882: recipe for target 'libxul.so.48.0' failed

    
    

    
  
This is mainly due to us switching to a new way to generate moz.build files from a gyp autogenerator, and the moz.build file overall has changed significantly as a result. Unfortunately, because of this, the patch in the patches directory is obsolete.

When you find out what changes need to be made for moz.build to work for your toolchain, I can help in getting the gyp_mozbuild script updated to cater for it.

    
    

    
  
ugh. scripts generating intermediate files generating build files. the horror.

anyway, it seems the SSSE3 file is added inconditionally (along the SSE2 files) in http://mxr.mozilla.org/mozilla-central/source/gfx/skia/generate_mozbuild.py#85. Maybe that part should be dependent on if CONFIG['INTEL_ARCHITECTURE'] and CONFIG['HAVE_TOOLCHAIN_SUPPORT_MSSSE3'] like it's done 20 lines before ?

    
    

    
  
That sounds reasonable to me

    
  
QA Whiteboard: [bugday-20140630]
Component: Untriaged → Build Config
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  
The reason for the cpuid check being ignored if the build machine has ssse3 is because -mssse3 gives the compiler discretion to use ssse3 instructions if it desires.

That hunk is unnecessary as we ensure that opts_check_SSE2.cpp is NOT built with mssse3. The rest looks fine, although the change to moz.build doesn't match the change in generate_mozbuild.py
Flags: needinfo?(gwright)

    
    

    
  
(In reply to George Wright (:gw280) from comment #12)
> The reason for the cpuid check being ignored if the build machine has ssse3
> is because -mssse3 gives the compiler discretion to use ssse3 instructions
> if it desires.
> 
> That hunk is unnecessary as we ensure that opts_check_SSE2.cpp is NOT built
> with mssse3. 

I'll have to retest without the chunk fiddling with disabled runtime cpuchecks, but i think stuart added it for a reason..

> The rest looks fine, although the change to moz.build doesn't
> match the change in generate_mozbuild.py

The change is not the same because generate_mozbuild.py is... a different beast, and i have a hard time figuring out how to make it generate something that would mimic what we hardcode in moz.build. Maybe removing _SSSE from http://mxr.mozilla.org/mozilla-central/source/gfx/skia/generate_mozbuild.py#253 and put the if block in generate_mozbuild.py's footer ?

    
    

    
  
either way, i dont see a way to run generate_mozbuild.py here on OpenBSD without having to checkout an svn revision from skia and going through insane loops(/bin/bash hardcoded, env python,..), so i can hardly try to fix generate_mozbuild.

    
    

    
  
with a simplified diff trying to address all issues without touching opts_check_sse2.cpp, i get more or less the same thing as in comment 7:

/usr/bin/ld: section .text [0031e770 -> 02d0c93b] overlaps section .data.rel.ro [003f3f58 -> 003f403f]
../../../gfx/skia/opts_check_SSE2.o(.text+0x198): In function `SkBitmapProcState::platformProcs()':
/src/mozilla-central/gfx/skia/trunk/src/opts/opts_check_SSE2.cpp:126: undefined reference to `S32_opaque_D32_filter_DX_SSSE3(SkBitmapProcState const&, unsigned int const*, int, unsigned int*)'
../../../gfx/skia/opts_check_SSE2.o(.text+0x1ad):/src/mozilla-central/gfx/skia/trunk/src/opts/opts_check_SSE2.cpp:128: undefined reference to `S32_alpha_D32_filter_DX_SSSE3(SkBitmapProcState const&, unsigned int const*, int, unsigned int*)'
../../../gfx/skia/opts_check_SSE2.o(.text+0x1c3):/src/mozilla-central/gfx/skia/trunk/src/opts/opts_check_SSE2.cpp:132: undefined reference to `S32_opaque_D32_filter_DXDY_SSSE3(SkBitmapProcState const&, unsigned int const*, int, unsigned int*)'
../../../gfx/skia/opts_check_SSE2.o(.text+0x1d8):/src/mozilla-central/gfx/skia/trunk/src/opts/opts_check_SSE2.cpp:134: undefined reference to `S32_alpha_D32_filter_DXDY_SSSE3(SkBitmapProcState const&, unsigned int const*, int, unsigned int*)'
/usr/bin/ld: libxul.so.1.0: hidden symbol `_Z31S32_alpha_D32_filter_DXDY_SSSE3RK17SkBitmapProcStatePKjiPj' isn't defined

    
    

    
  
libxul links if i readd the opts_check_sse2.cpp check...

    
    

    
  


  
Just for the record, here's what works for me. In the end, i grouped the SSSE3 bits a the same place in the footer, to make sure they are updated together. If someone could run generate_mozbuild.py for me and check it produces the correct result..

as for the opts_check_SSE2.cpp part, i'd like to discuss each chunk separately. The first bit returning true if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2 looks wrong to me, because (to me) that means 'if the build machine supports SSE2, the machine will support it' - that might be wrong with old i386 from the 2005 era.
The second bit is on the same boat for SSSE3 ? or that depends on -mssse3 ? 

The last bit is controversial, but i dont know how to make it link here without undef symbols, and without breaking platforms that support SSSE3.

        Attachment #8453672 -
        Flags: feedback?(gwright)

    
    

    
  
or make the whole block also depending on an SSSE3 ifdef.. to cope for when we dont *build* the ssse3 file

    
    

    
  
The SSSE3 files still need to be compiled even if your compiler doesn't support -mssse3. SK_CPU_SSE_LEVEL is set to _SSSE3 only if -mssse3 is used to compile that file (judging by the headers... unless somehow your compiler is defining __SSSE3__ without -mssse3 being specified, which it shouldn't afaik). If SK_CPU_SSE_LEVEL isn't high enough for the _SSSE3.cpp files then stub functions will be compiled in that don't call SSSE3 instructions.

As for the SSE2 chunk in opts_check_SSE2.cpp, it seems correct to me as my understanding is that SK_CPU_SSE_LEVEL will only be set as high as _SSE2 if and only if -msse2 is set on the command line, just like with SSSE3. The correct solution here is to ensure that opts_check_SSE2.cpp does not get compiled with either -msse2 or -mssse3. The reason for this is that the -msse flags allow the compiler to call those instructions when generating code so by definition that object file is not executable on a machine that doesn't have at least that SSE level. If that's the case, then we can just shortcut and return "true" in the functions that check for SSE level capabilities.

    
    

    
  
Comment on attachment 8453672 [details] [diff] [review]
simpler generate_mozbuild fix

Clearing flag as I've addressed the patch in a comment.

        Attachment #8453672 -
        Flags: feedback?(gwright)

    
    

    
  
All you should need to do here is to ensure that this line:

 SOURCES['trunk/src/opts/SkBitmapProcState_opts_SSSE3.cpp'].flags += ['-mssse3']

is conditional in a "CONFIG['INTEL_ARCHITECTURE'] and CONFIG['GNU_CC'] and CONFIG['HAVE_TOOLCHAIN_SUPPORT_MSSSE3']" block.

Do not remove the file from compilation or you will run into those linker issues, and everything else should just work.

    
    

    
  
okay, that now looks clearer to me. Thanks for the detailed explanation, will try only this in both the working (amd64) and non-working case (i386)

    
    

    
  
with a diff only setting -mssse3 if the toolchain supports it, build fails:


40:17.94 In file included from /src/mozilla-central/gfx/skia/trunk/src/opts/SkBitmapProcState_opts_SSSE3.cpp:19:0:
40:17.94 /usr/local/lib/gcc/i386-unknown-openbsd5.5/4.8.3/include/tmmintrin.h:31:3: error: #error "SSSE3 instruction set not enabled"
40:17.94  # error "SSSE3 instruction set not enabled"
40:17.94    ^
40:18.09 /src/mozilla-central/gfx/skia/trunk/src/opts/SkBitmapProcState_opts_SSSE3.cpp:50:49: error: '__m128i' does not name a type
40:18.09                                            const __m128i& mask_3FFF,

...
...

the full build line is:
eg++ -o SkBitmapProcState_opts_SSSE3.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /src/mozilla-central/config/gcc_hidden.h -DSK_A32_SHIFT=24 -DSK_R32_SHIFT=16 -DSK_G32_SHIFT=8 -DSK_B32_SHIFT=0 -DSK_USE_POSIX_THREADS=1 -DSK_FONTHOST_DOES_NOT_USE_FONTMGR=1 -DSKIA_IMPLEMENTATION=1 -DGR_IMPLEMENTATION=1 -DMOZ_GLUE_IN_PROGRAM -DNO_NSPR_10_SUPPORT -I/src/mozilla-central/gfx/skia -I. -I/src/mozilla-central/gfx/skia/trunk/include/config -I/src/mozilla-central/gfx/skia/trunk/include/core -I/src/mozilla-central/gfx/skia/trunk/include/effects -I/src/mozilla-central/gfx/skia/trunk/include/gpu -I/src/mozilla-central/gfx/skia/trunk/include/images -I/src/mozilla-central/gfx/skia/trunk/include/lazy -I/src/mozilla-central/gfx/skia/trunk/include/pathops -I/src/mozilla-central/gfx/skia/trunk/include/pipe -I/src/mozilla-central/gfx/skia/trunk/include/ports -I/src/mozilla-central/gfx/skia/trunk/include/utils -I/src/mozilla-central/gfx/skia/trunk/include/utils/mac -I/src/mozilla-central/gfx/skia/trunk/include/utils/win -I/src/mozilla-central/gfx/skia/trunk/include/views -I/src/mozilla-central/gfx/skia/trunk/src/core -I/src/mozilla-central/gfx/skia/trunk/src/gpu -I/src/mozilla-central/gfx/skia/trunk/src/gpu/effects -I/src/mozilla-central/gfx/skia/trunk/src/gpu/gl -I/src/mozilla-central/gfx/skia/trunk/src/image -I/src/mozilla-central/gfx/skia/trunk/src/lazy -I/src/mozilla-central/gfx/skia/trunk/src/opts -I/src/mozilla-central/gfx/skia/trunk/src/sfnt -I/src/mozilla-central/gfx/skia/trunk/src/utils -I/src/mozilla-central/gfx/skia/trunk/src/utils/android -I/src/mozilla-central/gfx/skia/trunk/src/utils/mac -I/src/mozilla-central/gfx/skia/trunk/src/utils/win -I../../dist/include -I/usr/obj/m-c/dist/include/nspr -I/usr/obj/m-c/dist/include/nss -I/usr/obj/m-c/dist/include -I/src/mozilla-central/modules/zlib/src -fPIC -I/usr/X11R6/include -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/SkBitmapProcState_opts_SSSE3.o.pp -I/usr/X11R6/include -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -I/usr/obj/m-c/dist/include/cairo -I/usr/local/include/pango-1.0 -I/usr/local/include/cairo -I/usr/X11R6/include/pixman-1 -I/usr/local/include/libpng16 -I/usr/X11R6/include -I/usr/local/include/harfbuzz -pthread -I/usr/local/include/glib-2.0 -I/usr/local/lib/glib-2.0/include -I/usr/local/include -I/usr/X11R6/include/freetype2 -I/usr/include -I/usr/X11R6/include/freetype2 -I/usr/X11R6/include -I/usr/X11R6/include/freetype2 -I/usr/include -DNDEBUG -DTRIMMED -g -O -fomit-frame-pointer -Wno-overloaded-virtual -Wno-unused-function -Wno-logical-op /src/mozilla-central/gfx/skia/trunk/src/opts/SkBitmapProcState_opts_SSSE3.cpp

No -mssse3 on the commandline, but no stubs build.

/usr/obj/m-c/config.status:    (''' HAVE_TOOLCHAIN_SUPPORT_MSSSE3 ''', r'''  '''),

My understanding is that SK_CPU_SSE_LEVEL ends up being >= SK_CPU_SSE_LEVEL_SSSE3 for some reason i dont understand, and it tries to build the real funcs. BUT the compiler itself wont output any __SSE__ : eg++ -dM -E - < /dev/null  | grep -i sse yields nothing.

$eg++ -msse2 -dM -E - < /dev/null  | grep -i sse
#define __SSE2__ 1
#define __SSE__ 1
$eg++ -mssse3 -dM -E - < /dev/null  | grep -i sse
#define __SSE2__ 1
#define __SSSE3__ 1
#define __SSE__ 1
#define __SSE3__ 1

given that SK_CPU_SSE_LEVEL is only set in http://mxr.mozilla.org/mozilla-central/source/gfx/skia/trunk/include/core/SkPreConfig.h#118 i dont see what can mess things up.

    
    

    
  
Aahaa. *cries*. I think i got something here, in _opts_SSSE3.cpp:

 13 /* With the exception of the Android framework we always build the SSSE3 functions
 14  * and enable the caller to determine SSSE3 support.  However for the Android framework
 15  * if the device does not support SSSE3 then the compiler will not supply the required
 16  * -mssse3 option needed to build this file, so instead we provide a stub implementation.
 17  */
 18 #if !defined(SK_BUILD_FOR_ANDROID_FRAMEWORK) || SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSSE3

Sooooooo the stub implem is built only on android framework AND if the framework DOESNT support ssse3. *sigh*. May i remove the first part of the #if here, or at least add ||!defined(__OpenBSD__) ?

    
    

    
  
Since the protocol seems how 'upstream patches first', will it make sense to bug upstream so that !defined(SK_BUILD_FOR_ANDROID_FRAMEWORK) is removed ? Or upstream doesnt care about building on hardware/environments where SSSE3 is missing (would be bad, since all the bits to detect it are here...)

Otherwise i'll keep my local patch forever..
Flags: needinfo?(gwright)

    
    

    
  
Please file a bug upstream for that.
Flags: needinfo?(gwright)

    
    

    
  
I'm also not opposed to adding a new file, something like SkBitmapProcState_opts_SSSE3_none.cpp, for the time being that just implements the stub functions, then building that when HAVE_TOOLCHAIN_SUPPORT_MSSSE3 is 0.

    
    

    
  
Why not, but that would be an awkward way of saying 'we dont want to modify upstream, so we go through hoops to work around that'... and the stubs would be in both files then ?

    
    

    
  
Upstream have removed the android-only check now: https://codereview.chromium.org/391693004

This no longer blocks the Skia update.

    
    

    
  
Oh, it got reverted. I'll take a look.

    
    

    
  
Woo, great upstream move. I'll make sure to test this asap, but that should technically be what i had in mind.

    
    

    
  
Can the upstream commit be backported in time for 34 now, or it should wait for a skia update in-tree ? It seems it got reverted again upstream ?

    
    

    
  
George, any idea for my question in comment 34 - what should i do now since upstream changed their mind again ?
Flags: needinfo?(gwright)

    
    

    
  
It looks like it landed again https://codereview.chromium.org/414033002
Flags: needinfo?(gwright)

    
    

    
  
And the change was pulled in in the latest Skia update to gecko, so you should be good to just land the moz.build and generator hunks of the patch.

    
    

    
  
Argh. Guess i'll have to apply the same magic to SSE4 ...

/src/mozilla-central/gfx/skia/trunk/src/opts/SkBlitRow_opts_SSE4_asm.S: Assembler messages:
/src/mozilla-central/gfx/skia/trunk/src/opts/SkBlitRow_opts_SSE4_asm.S:100: Error: no such instruction: `ptest %xmm7,%xmm1'
/src/mozilla-central/gfx/skia/trunk/src/opts/SkBlitRow_opts_SSE4_asm.S:119: Error: no such instruction: `pblendvb %xmm5,%xmm3'

    
    

    
  
so, should i reuse this bug to fix the 'lets inconditionally build SSE4 code regardless of toolchain support' issue within moz.build, or file a new one ? I guess i should use HAVE_TOOLCHAIN_SUPPORT_MSSE4_1 like it was done for HAVE_TOOLCHAIN_SUPPORT_MSSSE3 ..

    
    

    
  
Seems reasonable to me to re-use this bug.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug-1028827-sse3-sse4.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Hi,

hear is an updated patch against mozilla-central which does the 
following things:

 1) if HAVE_TOOLCHAIN_SUPPORT_MSSSE3 is set, 
    add "-mssse3" flag for SkBitmapProcState_opts_SSSE3.cpp.

 2) if HAVE_TOOLCHAIN_SUPPORT_MSSSE4_1 is set, 
 2.1) add "-msse4.1" flag for SkBlurImage_opts_SSE4.cpp
 2.2) define SK_BUILD_SSSE41 like it was done with SK_BUILD_SSSE3.
 2.3) include sse4 specific asm sources.

 3) Only define SS4 specific prototypes if SK_BUILD_SSSE41 is defined.

This will fix the build on OpenBSD/i386.

Regards,
Fabian

    
    

    
  
Note that the flag is named sse4.1, not ssse4.1 :)

    
    

    
  
Comment on attachment 8511690 [details] [diff] [review]
bug-1028827-sse3-sse4.patch

According to mozilla's policy, the src/opts/SkBlitRow_opts_SSE4.h change should be upstreamed first, but upstream doesnt have SK_BUILD_SSE* defines - so that would be a local change only ? or rather provide an empty S32A_Opaque_BlitRow32_SSE4_asm() function in a separate file ?

I have to admit i'm a bit lost in all those options, and since my i386 is down i cant easily test/work on it.. george, what do you think about it ?
Flags: needinfo?(gwright)

    
    

    
  
Hmm i'm new to this. Tell me how you want it,
and i will prepare / test a patch :)

    
    

    
  
Comment on attachment 8511690 [details] [diff] [review]
bug-1028827-sse3-sse4.patch

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

Please ensure that your moz.build changes are applied to generate_mozbuild.py as well.

Other than the inline comments, it looks fine to me, but you should run it through a full try run before doing anything.

::: gfx/skia/moz.build
@@ +916,5 @@
>      DEFINES['SK_BUILD_SSSE3'] = 1
> +    SOURCES['trunk/src/opts/SkBitmapProcState_opts_SSSE3.cpp'].flags += ['-mssse3']
> +
> +if CONFIG['INTEL_ARCHITECTURE'] and CONFIG['HAVE_TOOLCHAIN_SUPPORT_MSSSE4_1']:
> +    DEFINES['SK_BUILD_SSSE41'] = 1

I think you can safely remove this, as the -msse4.1 flag should just do the right thing, provided you change the #if in SkBlitRow_opts_SSE4.h as I have shown. The SK_BUILD_SSSE3 flag is a legacy thing that no longer does anything, so that can probably be removed too.

::: gfx/skia/trunk/src/opts/SkBlitRow_opts_SSE4.h
@@ +18,5 @@
>   */
>  #if /* 1)*/ (defined(__clang__) || (defined(__GNUC__) && !defined(SK_BUILD_FOR_MAC))) \
>      /* 2)*/ && !defined(SK_BUILD_FOR_WIN)                                             \
> +    /* 3)*/ && !defined(MEMORY_SANITIZER)                                             \
> +    /* 4)*/ && defined(SK_BUILD_SSE41)

#if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE41

        Attachment #8511690 -
        Flags: feedback+

    
    

    
  
As for upstream, just submit a patch to codereview with the #if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE41 patch (provided it works, I haven't tested it), and flag "reed1" for review. See information on submitting upstream here: https://sites.google.com/site/skiadocs/developer-documentation/contributing-code/how-to-submit-a-patch
Flags: needinfo?(gwright)

    
    

    
  
Also please CC me on the upstream bug so I can keep an eye on it.

    
    

    
  


  
Here's a new version which includes most of your feedback.

The -mssse3/-msse4.1 flags still causes trouble. This is due to our old toolchain i think? See error messages below. I left the HAVE_TOOLCHAIN_SUPPORT_MSSSE* conditionals but moved them above there original definition and removed "SK_BUILD_SSSE3" as you suggested.

This works for me for firefox-34.0beta2/i386 and mozilla-central/amd64.

Who can do a full try run? Landry?



Build errors when using -mssse3 / -mss4.1 on i386:

c++ -o SkBitmapProcState_opts_SSSE3.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /home/ports/obj/firefox-34.0beta2/mozilla-beta/
config/gcc_hidden.h -DSK_USE_POSIX_THREADS=1 -DSK_FONTHOST_DOES_NOT_USE_FONTMGR=1 -DSKIA_IMPLEMENTATION=1 -DGR_IMPLEMENTATION=1 -DMOZ_GLUE_IN_PROGRAM -D
AB_CD=en-US -DNO_NSPR_10_SUPPORT -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia -I. -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia
/trunk/include/config -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/core -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/
skia/trunk/include/effects -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/gpu -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/
gfx/skia/trunk/include/images -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/pathops -I/home/ports/obj/firefox-34.0beta2/mozill
a-beta/gfx/skia/trunk/include/pipe -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/ports -I/home/ports/obj/firefox-34.0beta2/moz
illa-beta/gfx/skia/trunk/include/utils -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/utils/mac -I/home/ports/obj/firefox-34.0b
eta2/mozilla-beta/gfx/skia/trunk/include/utils/win -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/views -I/home/ports/obj/firef
ox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/core -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/gpu -I/home/ports/obj/firefox-34.0
beta2/mozilla-beta/gfx/skia/trunk/src/gpu/effects -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/gpu/gl -I/home/ports/obj/firefox-3
4.0beta2/mozilla-beta/gfx/skia/trunk/src/image -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/lazy -I/home/ports/obj/firefox-34.0be
ta2/mozilla-beta/gfx/skia/trunk/src/opts -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/sfnt -I/home/ports/obj/firefox-34.0beta2/mo
zilla-beta/gfx/skia/trunk/src/utils -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/utils/mac -I/home/ports/obj/firefox-34.0beta2/mo
zilla-beta/gfx/skia/trunk/src/utils/win -I../../dist/include  -I/usr/local/include/nspr -I/usr/local/include/nss   -I/usr/include    -fPIC  -I/usr/X11R6
/include -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/SkBitmapProcState_opts_SSSE3.o.pp -I/usr/X11R6/include -Wall -Wpointer-arith
 -Woverloaded-virtual -Werror=return-type -Werror=int-to-pointer-cast -Werror=type-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align
 -O2 -pipe -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pip
e  -DNDEBUG -DTRIMMED -fno-omit-frame-pointer  -Wno-overloaded-virtual -Wno-unused-function -fomit-frame-pointer -Wno-logical-op -I/home/ports/obj/firef
ox-34.0beta2/build-i386/dist/include/cairo -I/usr/X11R6/include/freetype2 -I/usr/X11R6/include -I/usr/X11R6/include/freetype2 -I/usr/include -I/usr/loca
l/include/pango-1.0 -I/usr/local/include/cairo -I/usr/X11R6/include/pixman-1 -I/usr/local/include/libpng16 -I/usr/X11R6/include -I/usr/local/include/harfbuzz -pthread -I/usr/local/include/glib-2.0 -I/usr/local/lib/glib-2.0/include -I/usr/local/include -I/usr/X11R6/include/freetype2 -I/usr/include  -mssse3  /home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/opts/SkBitmapProcState_opts_SSSE3.cpp
{standard input}: Assembler messages:
{standard input}:101: Error: no such instruction: `pmaddubsw %xmm3,%xmm4'
{standard input}:113: Error: no such instruction: `pmaddubsw %xmm1,%xmm0'
{standard input}:157: Error: no such instruction: `pmaddubsw %xmm0,%xmm7'
{standard input}:250: Error: no such instruction: `pmaddubsw %xmm1,%xmm0'
{standard input}:262: Error: no such instruction: `pmaddubsw %xmm1,%xmm5'
{standard input}:276: Error: no such instruction: `pmaddubsw %xmm3,%xmm1'
{standard input}:287: Error: no such instruction: `pmaddubsw %xmm3,%xmm2'
{standard input}:335: Error: no such instruction: `pmaddubsw %xmm1,%xmm0'
{standard input}:339: Error: no such instruction: `pmaddubsw %xmm1,%xmm7'
{standard input}:461: Error: no such instruction: `pmaddubsw %xmm3,%xmm4'
{standard input}:475: Error: no such instruction: `pmaddubsw %xmm2,%xmm0'
{standard input}:520: Error: no such instruction: `pmaddubsw %xmm2,%xmm0'
{standard input}:615: Error: no such instruction: `pmaddubsw %xmm2,%xmm0'
{standard input}:627: Error: no such instruction: `pmaddubsw %xmm2,%xmm5'
{standard input}:644: Error: no such instruction: `pmaddubsw %xmm4,%xmm2'
{standard input}:655: Error: no such instruction: `pmaddubsw %xmm4,%xmm3'
{standard input}:703: Error: no such instruction: `pmaddubsw %xmm2,%xmm0'
{standard input}:707: Error: no such instruction: `pmaddubsw %xmm2,%xmm4'
{standard input}:832: Error: no such instruction: `pmaddubsw %xmm1,%xmm0'
{standard input}:847: Error: no such instruction: `pmaddubsw %xmm1,%xmm5'
{standard input}:912: Error: no such instruction: `pmaddubsw %xmm0,%xmm1'
{standard input}:920: Error: no such instruction: `pmaddubsw %xmm0,%xmm6'
{standard input}:1058: Error: no such instruction: `pmaddubsw %xmm1,%xmm0'
{standard input}:1071: Error: no such instruction: `pmaddubsw %xmm1,%xmm3'
{standard input}:1140: Error: no such instruction: `pmaddubsw %xmm1,%xmm0'
{standard input}:1146: Error: no such instruction: `pmaddubsw %xmm1,%xmm6'
/home/ports/obj/firefox-34.0beta2/mozilla-beta/config/rules.mk:948: recipe for target 'SkBitmapProcState_opts_SSSE3.o' failed
gmake[3]: *** [SkBitmapProcState_opts_SSSE3.o] Error 1



c++ -o SkBlurImage_opts_SSE4.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /home/ports/obj/firefox-34.0beta2/mozilla-beta/config/gcc_hidden.h -DSK_USE_POSIX_THREADS=1 -DSK_FONTHOST_DOES_NOT_USE_FONTMGR=1 -DSKIA_IMPLEMENTATION=1 -DGR_IMPLEMENTATION=1 -DMOZ_GLUE_IN_PROGRAM -DAB_CD=en-US -DNO_NSPR_10_SUPPORT -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia -I. -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/config -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/core -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/effects -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/gpu -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/images -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/pathops -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/pipe -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/ports -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/utils -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/utils/mac -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/utils/win -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/include/views -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/core -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/gpu -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/gpu/effects -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/gpu/gl -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/image -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/lazy -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/opts -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/sfnt -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/utils -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/utils/mac -I/home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/utils/win -I../../dist/include  -I/usr/local/include/nspr -I/usr/local/include/nss   -I/usr/include    -fPIC  -I/usr/X11R6/include -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/SkBlurImage_opts_SSE4.o.pp -I/usr/X11R6/include -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Werror=int-to-pointer-cast -Werror=type-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align -O2 -pipe -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe  -DNDEBUG -DTRIMMED -fno-omit-frame-pointer  -Wno-overloaded-virtual -Wno-unused-function -fomit-frame-pointer -Wno-logical-op -I/home/ports/obj/firefox-34.0beta2/build-i386/dist/include/cairo -I/usr/X11R6/include/freetype2 -I/usr/X11R6/include -I/usr/X11R6/include/freetype2 -I/usr/include -I/usr/local/include/pango-1.0 -I/usr/local/include/cairo -I/usr/X11R6/include/pixman-1 -I/usr/local/include/libpng16 -I/usr/X11R6/include -I/usr/local/include/harfbuzz -pthread -I/usr/local/include/glib-2.0 -I/usr/local/lib/glib-2.0/include -I/usr/local/include -I/usr/X11R6/include/freetype2 -I/usr/include  -msse4.1  /home/ports/obj/firefox-34.0beta2/mozilla-beta/gfx/skia/trunk/src/opts/SkBlurImage_opts_SSE4.cpp
{standard input}: Assembler messages:
{standard input}:87: Error: no such instruction: `pmulld %xmm1,%xmm0'
{standard input}:225: Error: no such instruction: `pmulld %xmm1,%xmm0'
{standard input}:371: Error: no such instruction: `pmulld %xmm1,%xmm0'
{standard input}:525: Error: no such instruction: `pmulld %xmm1,%xmm0'
/home/ports/obj/firefox-34.0beta2/mozilla-beta/config/rules.mk:948: recipe for target 'SkBlurImage_opts_SSE4.o' failed

        Attachment #8511690 -
        Attachment is obsolete: true

    
    

    
  
That's weird, and I'm not sure what the best way to fix that is. pmulld/pmaddubsw are in SSE4.1 and SSSE3 respectively, so if you're passing -msse4.1 and -mssse3 to gcc then it should have them available.

    
    

    
  
Oh, it's on i386 and not AMD64? Maybe that's the issue. Perhaps we need to disable SSSE3 and SSE4.1 on non-64bit?

    
    

    
  
Hmm, the errors are only present when unconditionally adding the -mssse3 -msse4.1 flags.
When using HAVE_TOOLCHAIN_SUPPORT_{MSSSE3,MSSSE4_1} to check if the compiler supports these flags like we do with this patch we should be save no?

    
    

    
  
(In reply to Fabian Raetz from comment #51)
> Hmm, the errors are only present when unconditionally adding the -mssse3
> -msse4.1 flags.
> When using HAVE_TOOLCHAIN_SUPPORT_{MSSSE3,MSSSE4_1} to check if the compiler
> supports these flags like we do with this patch we should be save no?

Oh yes, you should only ever add -mss{se3,e4.1} if the toolchain supports it. :)

    
    

    
  
That's exactly the original intent of this bug, preventing the build system to hardcode -mss{se3,e4.1} and make it conditional to os/toolchain support via HAVE_TOOLCHAIN_SUPPORT_{MSSSE3,MSSSE4_1}

-    SOURCES['trunk/src/opts/SkBlurImage_opts_SSE4.cpp'].flags += ['-msse4.1']
+    if CONFIG['HAVE_TOOLCHAIN_SUPPORT_MSSSE4_1']:
+        SOURCES['trunk/src/opts/SkBlurImage_opts_SSE4.cpp'].flags += ['-msse4.1']

-    SOURCES['trunk/src/opts/SkBitmapProcState_opts_SSSE3.cpp'].flags += ['-mssse3']
+    if CONFIG['HAVE_TOOLCHAIN_SUPPORT_MSSSE3']:
+        SOURCES['trunk/src/opts/SkBitmapProcState_opts_SSSE3.cpp'].flags += ['-mssse3']

Since the source tree layout requires the .h/.cpp files to be compiled/included anyway (and they provide a default empty implem, based on SK_CPU_SSE_LEVEL #defines  ?), we need to ensure the flag is passed only if the toolchain supports it.

Anyway, here's a try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7b9f19fc95e7

    
    

    
  
There's a bugzilla field for that (see also) but it doesnt take codereview urls.. shouldnt it be a skia ticket like i did for the ssse3 part ?

    
    

    
  
Current m-c tree not only fails to build on i386 without ssse3/sse41 because of the current horror in skia, but *also* in libvpx.

48:56.35 In the directory  /usr/obj/buildslave/mozilla-central/media/libvpx
48:56.35 The following command failed to execute properly:
48:56.35 egcc -o vp9_idct_intrin_ssse3.o -c -I../../dist/system_wrappers -include /home/buildslave-i386/mozilla-central-i386/build/config/gcc_hidden.h -DHAVE_CONFIG_H=vpx_config.h -DMOZ_GLUE_IN_PROGRAM -DAB_CD=en-US -DNO_NSPR_10_SUPPORT -I/home/buildslave-i386/mozilla-central-i386/build/media/libvpx -I. -I../../dist/include -I/usr/obj/buildslave/mozilla-central/dist/include/nspr -I/usr/obj/buildslave/mozilla-central/dist/include/nss -fPIC -I/usr/X11R6/include -include ../../mozilla-config.h -DMOZILLA_CLIENT -MD -MP -MF .deps/vp9_idct_intrin_ssse3.o.pp -I/usr/X11R6/include -Wall -Wdeclaration-after-statement -Wempty-body -Wpointer-to-int-cast -Wsign-compare -Wtype-limits -Wno-unused -Wcast-align -std=gnu99 -fgnu89-inline -fno-strict-aliasing -fno-math-errno -pthread -pipe -DNDEBUG -DTRIMMED -g -O -fomit-frame-pointer -Wno-sign-compare -mssse3 /home/buildslave-i386/mozilla-central-i386/build/media/libvpx/vp9/common/x86/vp9_idct_intrin_ssse3.c
48:56.35 /home/buildslave-i386/mozilla-central-i386/build/config/rules.mk:898: recipe for target 'vp9_idct_intrin_ssse3.o' failed
48:56.35 gmake[5]: *** [vp9_idct_intrin_ssse3.o] Error 1

Of course, -mssse3 should be conditional here. But libvpx has such a spaghetti build system..

Using --with-system-libvpx is a temporary workaround.

    
    

    
  
That last build failure looks related to 2013f538837c / bug 1074860.

    
    

    
  
Readding the check for toolchain support :

-            if 'ssse3' in f:
+            if 'ssse3' in f and CONFIG['HAVE_TOOLCHAIN_SUPPORT_MSSSE3']:
                 SOURCES[f].flags += ['-mssse3']

doesnt fix this failure, because the sse2/ssse3 files are built inconditionally, and dont provide fallback when support is not here.

49:10.14 In file included from /src/mozilla-central/media/libvpx/vp9/common/x86/vp9_idct_intrin_ssse3.c:16:0:
49:10.14 /usr/local/lib/gcc/i386-unknown-openbsd5.6/4.8.3/include/tmmintrin.h:31:3: error: #error "SSSE3 instruction set not enabled"
49:10.14  # error "SSSE3 instruction set not enabled"
49:10.14    ^
49:10.14 In file included from /src/mozilla-central/media/libvpx/vp9/common/x86/vp9_idct_intrin_sse2.h:12:0,
49:10.14                  from /src/mozilla-central/media/libvpx/vp9/common/x86/vp9_idct_intrin_ssse3.c:17:
49:10.14 /usr/local/lib/gcc/i386-unknown-openbsd5.6/4.8.3/include/emmintrin.h:31:3: error: #error "SSE2 instruction set not enabled"
49:10.14  # error "SSE2 instruction set not enabled"
Blocks: 1074860
Blocks: 1122745

    
    

    
  
And now this affects 37/mozilla-beta, failing on libvpx. Yay.

    
  
Product: Core → Firefox Build System

    
  
Severity: normal → S3

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: