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

Status: NEW

(Firefox Build System :: General, defect)

Description

[stu-mozilla-persona]

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

Comment 1

[Landry Breuil (:gaston)]

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.

Comment 2

[stu-mozilla-persona]

This was broken in the skia update http://hg.mozilla.org/mozilla-central/rev/f6c2efda0788

Comment 3

[stu-mozilla-persona]

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,

Comment 4

[stu-mozilla-persona]

Attachment: skia-moz.build.diff

Comment 5

[Landry Breuil (:gaston)]

Probably related to 1023732 too...

Comment 6

[stu-mozilla-persona]

Attachment: skia-moz.build.diff

update; add missing piece

Comment 7

[stu-mozilla-persona]

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

Comment 8

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

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.

Comment 9

[Landry Breuil (:gaston)]

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 ?

Comment 10

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

That sounds reasonable to me

Comment 11

[Landry Breuil (:gaston)]

Fwiw, this got fixed locally with 3 patches:
http://www.openbsd.org/cgi-bin/cvsweb/ports/www/mozilla-firefox/patches/patch-gfx_skia_generate_mozbuild_py?rev=1.1
http://www.openbsd.org/cgi-bin/cvsweb/ports/www/mozilla-firefox/patches/patch-gfx_skia_moz_build?rev=1.1
http://www.openbsd.org/cgi-bin/cvsweb/ports/www/mozilla-firefox/patches/patch-gfx_skia_trunk_src_opts_opts_check_SSE2_cpp?rev=1.1

I'll have to reread it to make sure i understand all the bits of the change, but what do you think about it ?

Comment 12

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

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

Comment 13

[Landry Breuil (:gaston)]

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

Comment 14

[Landry Breuil (:gaston)]

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.

Comment 15

[Landry Breuil (:gaston)]

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

Comment 16

[Landry Breuil (:gaston)]

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

Comment 17

[Landry Breuil (:gaston)]

Attachment: simpler generate_mozbuild fix

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.

Comment 18

[Landry Breuil (:gaston)]

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

Comment 19

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

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 20

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

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

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

Comment 21

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

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.

Comment 22

[Landry Breuil (:gaston)]

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)

Comment 23

[Landry Breuil (:gaston)]

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.

Comment 24

[Landry Breuil (:gaston)]

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__) ?

Comment 25

[Landry Breuil (:gaston)]

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

Comment 26

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

Please file a bug upstream for that.

Comment 27

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

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.

Comment 28

[Landry Breuil (:gaston)]

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 ?

Comment 29

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

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

This no longer blocks the Skia update.

Comment 30

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

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

Comment 31

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

https://tbpl.mozilla.org/?tree=Try&rev=ede9dd963ccf

Comment 32

[Landry Breuil (:gaston)]

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

Comment 33

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

Fix landed for real now: https://codereview.chromium.org/403583002

Comment 34

[Landry Breuil (:gaston)]

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 ?

Comment 35

[Landry Breuil (:gaston)]

George, any idea for my question in comment 34 - what should i do now since upstream changed their mind again ?

Comment 36

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

It looks like it landed again https://codereview.chromium.org/414033002

Comment 37

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

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.

Comment 38

[Landry Breuil (:gaston)]

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'

Comment 39

[Landry Breuil (:gaston)]

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

Comment 40

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

Seems reasonable to me to re-use this bug.

Comment 41

[Fabian Raetz]

Attachment: bug-1028827-sse3-sse4.patch

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

Comment 42

[Landry Breuil (:gaston)]

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

Comment 43

[Landry Breuil (:gaston)]

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 ?

Comment 44

[Fabian Raetz]

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

Comment 45

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

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

Comment 46

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

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

Comment 47

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

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

Comment 48

[Fabian Raetz]

Attachment: bug-1028827-sse3-sse4_v2.patch

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

Comment 49

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

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.

Comment 50

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

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?

Comment 51

[Fabian Raetz]

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?

Comment 52

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

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

Comment 53

[Landry Breuil (:gaston)]

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

Comment 54

[Fabian Raetz]

Link to upstream issue: https://codereview.chromium.org/673903006/

Comment 55

[Landry Breuil (:gaston)]

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 ?

Comment 56

[Landry Breuil (:gaston)]

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.

Comment 57

[Landry Breuil (:gaston)]

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

Comment 58

[Landry Breuil (:gaston)]

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"

Comment 59

[Landry Breuil (:gaston)]

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