Bug 196488 – [CMake] Detect SSE2 at compile time

• Component: CMake
• Status: RESOLVED FIXED

[Xan Lopez]

This gets rid of a (bogus) static_assert, inert until now because of a typo fixed in bug #196204, and adds a simple CMake macro to detect SSE2 support.

[Xan Lopez]

Created attachment 366483 [details]
Detect SSE2 with CMake

[EWS Watchlist]

Attachment 366483 [details] did not pass style-queue:


ERROR: Source/cmake/FindSSE2.cmake:48:  No space between command "return" and its parentheses, should be "return("  [whitespace/parentheses] [5]
ERROR: Source/cmake/FindSSE2.cmake:52:  One space between command "if" and its parentheses, should be "if ("  [whitespace/parentheses] [5]
ERROR: Source/cmake/FindSSE2.cmake:53:  One space between command "if" and its parentheses, should be "if ("  [whitespace/parentheses] [5]
ERROR: Source/cmake/FindSSE2.cmake:55:  One space between command "endif" and its parentheses, should be "endif ("  [whitespace/parentheses] [5]
ERROR: Source/cmake/FindSSE2.cmake:56:  One space between command "elseif" and its parentheses, should be "elseif ("  [whitespace/parentheses] [5]
ERROR: Source/cmake/FindSSE2.cmake:57:  One space between command "if" and its parentheses, should be "if ("  [whitespace/parentheses] [5]
ERROR: Source/cmake/FindSSE2.cmake:60:  One space between command "endif" and its parentheses, should be "endif ("  [whitespace/parentheses] [5]
ERROR: Source/cmake/FindSSE2.cmake:61:  One space between command "endif" and its parentheses, should be "endif ("  [whitespace/parentheses] [5]
Total errors found: 8 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xan Lopez]

Created attachment 366485 [details]
Detect SSE2 with CMake

[Xan Lopez]

Created attachment 366492 [details]
Detect SSE2 with CMake

[Michael Catanzaro]

Comment on attachment 366485 [details]
Detect SSE2 with CMake

View in context: https://bugs.webkit.org/attachment.cgi?id=366485&action=review

> CMakeLists.txt:123
> +#---------------------------
> +# Make sure SSE2 is present.
> +#---------------------------
> +include(FindSSE2)
> +if (NOT SSE2_SUPPORT_FOUND)
> +    message(FATAL_ERROR "SSE2 support is required to compile WebKit")
> +endif ()

MIPS bot is failing. This should be guarded by if (WTF_CPU_X86) right?

[Xan Lopez]

(In reply to Michael Catanzaro from comment #5)
> 
> MIPS bot is failing. This should be guarded by if (WTF_CPU_X86) right?

Yeah, this was already fixed in a new patch but forgot to mark the previous one as obsolete.

[WebKit Commit Bot]

Comment on attachment 366492 [details]
Detect SSE2 with CMake

Clearing flags on attachment: 366492

Committed r243989: <https://trac.webkit.org/changeset/243989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Something seems to be going wrong with the CMake check and the CMake build system is not adding required flags for SSE2, so we'll need to try again.

There is more discussion in: https://gitlab.gnome.org/GNOME/gnome-build-meta/merge_requests/277

[Michael Catanzaro]

Reverted r243989 for reason:

Broke i686 builds

Committed r244138: <https://trac.webkit.org/changeset/244138>

[Konstantin Tokarev]

Comment on attachment 366492 [details]
Detect SSE2 with CMake

View in context: https://bugs.webkit.org/attachment.cgi?id=366492&action=review

> CMakeLists.txt:123
> +        message(FATAL_ERROR "SSE2 support is required to compile WebKit")

This is wrong, we should switch to CLoop if SSE2 is not supported, otherwise add -msse2 to cflags

[Xan Lopez]

(In reply to Konstantin Tokarev from comment #11)
> Comment on attachment 366492 [details]
> Detect SSE2 with CMake
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366492&action=review
> 
> > CMakeLists.txt:123
> > +        message(FATAL_ERROR "SSE2 support is required to compile WebKit")
> 
> This is wrong, we should switch to CLoop if SSE2 is not supported, otherwise
> add -msse2 to cflags

The flags are added in webkitdirs.pm. We are effectively requriring SSE2 to run WebKit now (see bug #194853 and the thread in webkit-dev linked from there). I think Michael's build error comes from the fact that distros do not use webkitdirs.pm, so they should be adding the SSE2 flags in their non x86-64 builds? Unless we want to do this ourselves somewhere else?

[Xan Lopez]

(In reply to Xan Lopez from comment #12)
> The flags are added in webkitdirs.pm. We are effectively requriring SSE2 to
> run WebKit now (see bug #194853 and the thread in webkit-dev linked from
> there). I think Michael's build error comes from the fact that distros do
> not use webkitdirs.pm, so they should be adding the SSE2 flags in their non
> x86-64 builds? Unless we want to do this ourselves somewhere else?

Seems WebkitCompilerFlags.cmake could be a good place to add the flags for x86, so I'll re-upload the patch with this tomorrow.

[Konstantin Tokarev]

(In reply to Xan Lopez from comment #12)
> (In reply to Konstantin Tokarev from comment #11)
> > Comment on attachment 366492 [details]
> > Detect SSE2 with CMake
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=366492&action=review
> > 
> > > CMakeLists.txt:123
> > > +        message(FATAL_ERROR "SSE2 support is required to compile WebKit")
> > 
> > This is wrong, we should switch to CLoop if SSE2 is not supported, otherwise
> > add -msse2 to cflags
> 
> The flags are added in webkitdirs.pm. We are effectively requriring SSE2 to
> run WebKit now (see bug #194853 and the thread in webkit-dev linked from
> there). 

My point is that we should't require SSE2 because we don't have to use JIT. Or this check should be guarget by if(ENABLE_JIT AND WTF_CPU_X86), which may align better with our policy of failing eagerly instead of degrading features silently based on system detection

[Konstantin Tokarev]

Distros indeed don't use webkitdirs.pm, AFAIK build-webkit and related files aren't even included into tarball

[Xan Lopez]

(In reply to Konstantin Tokarev from comment #14)
> My point is that we should't require SSE2 because we don't have to use JIT.
> Or this check should be guarget by if(ENABLE_JIT AND WTF_CPU_X86), which may
> align better with our policy of failing eagerly instead of degrading
> features silently based on system detection

This is also relevant for LLInt, even if you use CLoop, because the compiler flags you use to build LLInt/CLoop matter. Compiling it without SSE2 will effectively introduce "bugs" related to floating point precision.

In any case computers that do not have SSE2 capabilities will almost certainly not be able to run a current WebKit, I'd say. We are talking about almost 20 years old hardware.

Feel free to re-open the thread in webkit-dev if you want though, I think it's better to have the conversation there.

[Konstantin Tokarev]

>Compiling it without SSE2 will effectively introduce "bugs" related to floating point precision.

Precision of x87 calculations is more or equal than SSE2 because the former uses 80-bit registers and the latter - 64-bit. However, if we dig down that deep we will inevitably come to conclusion that all non-x86 platforms are all "buggy" (no)

[Xan Lopez]

(In reply to Konstantin Tokarev from comment #17)
> >Compiling it without SSE2 will effectively introduce "bugs" related to floating point precision.
> 
> Precision of x87 calculations is more or equal than SSE2 because the former
> uses 80-bit registers and the latter - 64-bit. However, if we dig down that
> deep we will inevitably come to conclusion that all non-x86 platforms are
> all "buggy" (no)

That's why I wrote "bugs", not bugs. The issue is that those differences do add up, and differences in rounding lead to strange failures that take a long time to figure out. So I proposed to just use SSE2 everywhere, which is what Firefox and Chrome are already doing, and save ourselves some time in the future. In any case, again, feel free to comment on the thread, I did make a public RFC in webkit-dev.

(In reply to Xan Lopez from comment #16)
> In any case computers that do not have SSE2 capabilities will almost
> certainly not be able to run a current WebKit, I'd say. We are talking about
> almost 20 years old hardware.
> 

Well, I was using WebKit until now, but since the latest release it is broken and I cannot start my browser at all (GNOME Web (epiphany-browser)).

I have an Athlon XP running Lubuntu 18 and I was using WebKit. I had to fix WebKit last year because it required SSE2: https://bugs.webkit.org/show_bug.cgi?id=188145

Since my fix I was using WebKit, but now I'm unable to use it again.

My computer is not older than 20 years, the CPU got onto the market in December 2004, so it is not even 15 years old yet.

Every other program I use on Linux runs fine without SSE2 (VLC, LibreOffice, whatever else). Isn't there an other solution than making SSE2 a requirement?

I've chosen WebKit because all the other browsers are requiring SSE2. If I cannot use WebKit then the latest browser with the most features is NetSurf compiled from git sources. It doesn't even capable of handling simpler JavaScripts in practice (there is only theoretic support). In contrast, with WebKit I could even watch YouTube videos.

[Pablo Saavedra]

The removed `isSSE2Present()`function is still present in assertions  included in MacroAssemblerX86Common.h.


Those assertions are reached trying to compile WPE for PC  using gcc 7.3.0:



```
In file included from DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:32:0,
                 from DerivedSources/ForwardingHeaders/wtf/FastMalloc.h:25,
                 from ../Source/JavaScriptCore/config.h:37,
                 from ../Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:26:
../Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h: In member function ‘void JSC::MacroAssemblerX86Common::moveDouble(JSC::AbstractMacroAssembler<JSC::X86Assembler>::FPRegisterID, JSC::AbstractMacroAssembler<JSC::X86Assembler>::FPRegisterID)’:
../Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:1458:16: error: ‘isSSE2Present’ was not declared in this scope
         ASSERT(isSSE2Present());
                ^
...
```

[Michael Catanzaro]

This commit was rolled out. I guess I'll add the rollout to the backports list for 2.24.

[Xan Lopez]

Created attachment 368924 [details]
Detect SSE2 with CMake

New patch.

The SSE2 flags are now added in WebKitCompilerFlags.cmake. We try to detect SSE2 right after (with the flags in place). webkitdirs.pm has been modified to not do this anymore, since it would be redundant.

This works locally, but if Michael or someone could give it a shot before pushing again that would be nice.

[Xan Lopez]

Created attachment 368925 [details]
Detect SSE2 with CMake

And now make it apply to ToT.

[EWS Watchlist]

Attachment 368925 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xan Lopez]

Created attachment 368926 [details]
Detect SSE2 with CMake

Style fix.

[Konstantin Tokarev]

Comment on attachment 368926 [details]
Detect SSE2 with CMake

View in context: https://bugs.webkit.org/attachment.cgi?id=368926&action=review

> Source/cmake/FindSSE2.cmake:52
> +    if (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_GNUCXX OR CMAKE_COMPILER_IS_CLANG)

Should be if (COMPILER_IS_GCC_OR_CLANG)

> Source/cmake/WebKitCompilerFlags.cmake:149
> +    if (WTF_CPU_X86 AND NOT WIN32)

Should be if (WTF_CPU_X86 AND COMPILER_IS_GCC_OR_CLANG)

[Michael Catanzaro]

Comment on attachment 368926 [details]
Detect SSE2 with CMake

View in context: https://bugs.webkit.org/attachment.cgi?id=368926&action=review

> Source/cmake/WebKitCompilerFlags.cmake:150
> +        WEBKIT_PREPEND_GLOBAL_COMPILER_FLAGS(-march=pentium4 -msse2 -mfpmath=sse)

Is -march=pentium4 required? I think distros usually use -march=generic.

[EWS Watchlist]

Comment on attachment 368926 [details]
Detect SSE2 with CMake

Attachment 368926 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12093170

New failing tests:
svg/repaint/change-background-color.html
legacy-animation-engine/fast/layers/no-clipping-overflow-hidden-hardware-acceleration.html

[EWS Watchlist]

Created attachment 369023 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Konstantin Tokarev]

Comment on attachment 368926 [details]
Detect SSE2 with CMake

View in context: https://bugs.webkit.org/attachment.cgi?id=368926&action=review

>> Source/cmake/WebKitCompilerFlags.cmake:150
>> +        WEBKIT_PREPEND_GLOBAL_COMPILER_FLAGS(-march=pentium4 -msse2 -mfpmath=sse)
> 
> Is -march=pentium4 required? I think distros usually use -march=generic.

It is not required, and may introduce a regression if someone uses toolchain with custom default -march and relies on that default value

[Xan Lopez]

(In reply to Michael Catanzaro from comment #27)
> Comment on attachment 368926 [details]
> Detect SSE2 with CMake
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368926&action=review
> 
> > Source/cmake/WebKitCompilerFlags.cmake:150
> > +        WEBKIT_PREPEND_GLOBAL_COMPILER_FLAGS(-march=pentium4 -msse2 -mfpmath=sse)
> 
> Is -march=pentium4 required? I think distros usually use -march=generic.

-march=generic does not exist, and -mtune does not make sense here. That line has been around forever, but I assume the intent was to explicitly limit the available instruction set. I'll leave it out.

[Xan Lopez]

(In reply to Konstantin Tokarev from comment #26)
> 
> > Source/cmake/WebKitCompilerFlags.cmake:149
> > +    if (WTF_CPU_X86 AND NOT WIN32)
> 
> Should be if (WTF_CPU_X86 AND COMPILER_IS_GCC_OR_CLANG)

The whole block is already inside an if (COMPILER_IS_GCC_OR_CLANG), so I assume WIN32 is not necessary?

[Xan Lopez]

Created attachment 369092 [details]
Detect SSE2 with CMake

New patch.

[EWS Watchlist]

Comment on attachment 369092 [details]
Detect SSE2 with CMake

Attachment 369092 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12105974

New failing tests:
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html

[EWS Watchlist]

Created attachment 369093 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Konstantin Tokarev]

Comment on attachment 369092 [details]
Detect SSE2 with CMake

View in context: https://bugs.webkit.org/attachment.cgi?id=369092&action=review

> Source/cmake/FindSSE2.cmake:50
> +        HAVE_SSE2_EXTENSIONS)

This test may fail in cross-compilation. Shouldn't it use check_cxx_source_compiles()?

> Source/cmake/WebKitCompilerFlags.cmake:151
> +        include(FindSSE2)

find_package(SSE2 REQUIRED)

Note that to make REQUIRED work you should also add FIND_PACKAGE_HANDLE_STANDARD_ARGS to FindSSE2.cmake

[Xan Lopez]

(In reply to Konstantin Tokarev from comment #36)
> Comment on attachment 369092 [details]
> Detect SSE2 with CMake
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369092&action=review
> 
> > Source/cmake/FindSSE2.cmake:50
> > +        HAVE_SSE2_EXTENSIONS)
> 
> This test may fail in cross-compilation. Shouldn't it use
> check_cxx_source_compiles()?

The build script used to leave this alone when crosscompiling, which honestly makes sense to me. So I've added an extra guard for this in the first if() check, let me know what you think.

> 
> > Source/cmake/WebKitCompilerFlags.cmake:151
> > +        include(FindSSE2)
> 
> find_package(SSE2 REQUIRED)
> 
> Note that to make REQUIRED work you should also add
> FIND_PACKAGE_HANDLE_STANDARD_ARGS to FindSSE2.cmake

My impression is that for the cmake modules WebKit ships include() is used, while find_package() is used for external libraries. So find_package() seems overkill?

[Xan Lopez]

Created attachment 369115 [details]
Detect SSE2 with CMake

New patch.

[EWS Watchlist]

Comment on attachment 369115 [details]
Detect SSE2 with CMake

Attachment 369115 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12110076

New failing tests:
svg/repaint/remove-border-property-on-root.html

[EWS Watchlist]

Created attachment 369118 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Michael Catanzaro]

Suggesting find_package() to find something that's not a package seems like a design problem.

[Konstantin Tokarev]

If we don't want to use find_package, module name should not start with Find*

[Konstantin Tokarev]

(In reply to Xan Lopez from comment #37)
> (In reply to Konstantin Tokarev from comment #36)
> > Comment on attachment 369092 [details]
> > Detect SSE2 with CMake
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=369092&action=review
> > 
> > > Source/cmake/FindSSE2.cmake:50
> > > +        HAVE_SSE2_EXTENSIONS)
> > 
> > This test may fail in cross-compilation. Shouldn't it use
> > check_cxx_source_compiles()?
> 
> The build script used to leave this alone when crosscompiling, which
> honestly makes sense to me. So I've added an extra guard for this in the
> first if() check, let me know what you think.

It still makes sense to check that target compiler supports SSE2 intrinsics. OTOH, I see little sense in checking that SSE2 is available on CPU where WebKit is compiled, because it's not necessary the machine where build products are meant to run on.

> 
> > 
> > > Source/cmake/WebKitCompilerFlags.cmake:151
> > > +        include(FindSSE2)
> > 
> > find_package(SSE2 REQUIRED)
> > 
> > Note that to make REQUIRED work you should also add
> > FIND_PACKAGE_HANDLE_STANDARD_ARGS to FindSSE2.cmake
> 
> My impression is that for the cmake modules WebKit ships include() is used,
> while find_package() is used for external libraries. So find_package() seems
> overkill?

No, we consistently use find_package() for Find modules inside Source/cmake

[Konstantin Tokarev]

> OTOH, I see little sense in checking that SSE2 is available on CPU where
> WebKit is compiled, because it's not necessary the machine where build
> products are meant to run on.

Though run check may be useful for DEVELOPER_MODE

[Xan Lopez]

Created attachment 369473 [details]
Detect SSE2 with CMake

New patch.
- Rename FindSSE2.cmake to DetectSSE2.cmake, since this is not trying to find any package in any meaningful way.
- Do not run the SSE2 check when crosscompiling, since the building machine won't be the one running the binary anyway (this is what build-jsc does/did too).

[Xan Lopez]

Created attachment 369476 [details]
Detect SSE2 with CMake

Fix weird path arrangement in commit message.

[EWS Watchlist]

Comment on attachment 369476 [details]
Detect SSE2 with CMake

Attachment 369476 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12141961

New failing tests:
http/tests/css/filters-on-iframes.html

[EWS Watchlist]

Created attachment 369481 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[WebKit Commit Bot]

Comment on attachment 369476 [details]
Detect SSE2 with CMake

Clearing flags on attachment: 369476

Committed r245127: <https://trac.webkit.org/changeset/245127>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

> So I proposed to just use SSE2 everywhere, which is
> what Firefox and Chrome are already doing, and save ourselves some time in
> the future. In any case, again, feel free to comment on the thread, I did
> make a public RFC in webkit-dev.

Just for the record - for my greatest surprise - Firefox 67 works without SSE2. I've just watched a YouTube video at 144p on my Athlon XP 2600+.