1290932 - Build the JavaScript engine with -msse2 -mfpmath=sse under the ARM simulator build

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

TL;DR: WebAssembly supports signaling NaN (sNaN) and having them as observable values. To support moving sNaN as function arguments and return values, we need -msse2 -mfpmath=sse. Could we use this for compiling the shell?

The long story: wasm offers the feature of having custom payloads in NaN numbers. To be compatible with how most processors work, almost every single instruction operating on a sNaN transforms the sNaN into a quiet NaN (qNaN) (e.g. sNaN + 0 === qNaN). But sNaN can still be observed, for instance by storing their value into the wasm linear memory (heap) and then reading bytes from the heap at this location.

On x86 with the current build flags, the C++ calling convention passes float arguments using the x87 floating-point stack (on x86, that would use the fld/flds/fstp/fstps instructions). These instructions have the side-effect of transforming sNaN into qNaN, which is not desired. Return values are also passed through the x87 FP stack, with the same instructions.

So in our code, when wasm::ParseNaNLiteral calls mfbt's SpecificNaN [0] and retrieves the return value (by value, in opposition to "by outparam"), the passed NaN is not a sNaN, but a qNaN already.

Fortunately, if we compile with -msse2 -mfpmath=sse, then the compiler will pass arguments by not using the x87 FP stack. Return values are still using the FP stack; but every function that is called in this code path can just use outparams, so that is enough for our use-case. (for return values, returning by const& does the trick)

v8/chromium seems to use these compilation flags already [1] [2], as it is apparent in our inclusions of google's code [3].

So can we use this too for compiling the JS shell, at least? If so, can you tell me where to change the compiler flags on all the configurations, or steal the task if you have some cycles, please?

[0] http://searchfox.org/mozilla-central/source/js/src/asmjs/WasmTextToBinary.cpp#1572
[1] https://github.com/v8/v8/blob/master/BUILD.gn#L304
[2] https://github.com/v8/v8/blob/8558cbe557c5cd5f8a284743f8db48668afc4266/gypfiles/toolchain.gypi#L1013-L1014
[3] http://searchfox.org/mozilla-central/source/media/webrtc/trunk/webrtc/common_audio/BUILD.gn#196

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Needinfo for the question in the TLDR, or at the bottom of comment 0.

Comment 2

[Gregory Szorc [:gps]]

Search for "-msse" in old-configure.in.

It looks like js/src's configure is out of sync with Gecko's, which is presumably why js/src isn't getting -msse* flags. We'll probably want to move the SSE configure code to moz.configure as part of making it work for js/src (we have to refactor the code to make it shared, so we might as well port to moz.configure if possible). glandium is neck deep in moz.configure porting work, so I'll wait for him to weigh in. He may even assign this bug to himself :)

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

I've been staring at the /old-configure.in file for a bit long now; from what I understand, the -msse flags are saved in a SSE_FLAGS variable, that is then used to build only certain files from the tree:

http://searchfox.org/mozilla-central/source/gfx/qcms/moz.build#41
http://searchfox.org/mozilla-central/source/gfx/cairo/libpixman/src/moz.build#124

My m4 foo is non-existent, so I am probably just going to wait on the porting work, unless this happens after 51 (we'd like to ship the feature blocked by this bug before 51, ideally).

Comment 4

[Mike Hommey [:glandium]]

Those SSE_FLAGS in old-configure.in have nothing to do with whether we require sse2 for Firefox (which, incidentally, we don't yet, that's bug 1274196), and even if we didn, we wouldn't require it for non-Mozilla builds. IOW, You can't assume everyone is going to build for x86 with sse2. It's also potentially dangerous to assume that all tier-3 architectures where Firefox currently runs have the same property as x86 with -msse2 -mfpmath=sse wrt preserving sNaN.

I'd say, if you want to stick bits into a floating point value and want those bits to be preserved, you shouldn't be manipulating a floating point value, but an integer value that happens to have the same bits as a floating point value.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Dan, are there other platforms that may have this kind of behavior? My testing on ARMv6 (raspberry pi 2 model b) showed no errors on the tests remaining in the blocked bug.

For what it's worth, the alternative option comment 4 mentions is much more intrusive a change, very error prone and a road it'd be better to avoid, in my opinion; I'd started implementing it this way, before realizing that almost all the parts of the JS engine would need to handle the exceptional float-stored-as-int32 path...

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Setting needinfo for question in first paragraph of comment 5.

Comment 7

[Dan Gohman [:sunfish]]

No, I'm not aware of any other relevant platforms where load/store convert sNaN to qNaN or perform any other conversion.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

What is the benefit of the proposed change?

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #8)
> What is the benefit of the proposed change?

Preserving the signaling/quiet bit on NaN with custom payloads (when passing such a NaN value as a parameter to a call), which **is a WebAssembly feature** (so on all platforms, x86/x64/ARM). Here, x86 is an issue because of its calling convention (at least under linux, it tries to pass floating-point arguments on the FP x87 stack, which resets the signaling/quiet bit).

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Mike, let's try to find out a solution.

WebAssembly won't work at all without SSE2: there is a dynamic check that SSE2 is enabled at runtime, preventing compilation if we don't have SSE2.

I am wondering if we could just *try* to use these flags on x86. If they work, we use them for building the JS engine (the rest of gecko doesn't have to depend upon this); if they don't, that is fine, we can just compile like we do today and WebAssembly won't work (the hypothesis being that if these flags don't work, there's no SSE2 on the machine; I have no data backing this hypothesis up though, but at least it sounds plausible).

Would that work for you? Other platforms shouldn't have this issue, according to previous comments (in particular comment 7).

If so, can you please tell me how I could implement this? (or even better, stealing it would be awesome :))
I can obviously help by e.g. crafting a C++ small test program that checks whether a sNaN is preserved when passed as a parameter.

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> Preserving the signaling/quiet bit on NaN with custom payloads (when passing
> such a NaN value as a parameter to a call), which **is a WebAssembly
> feature** (so on all platforms, x86/x64/ARM). Here, x86 is an issue because
> of its calling convention (at least under linux, it tries to pass
> floating-point arguments on the FP x87 stack, which resets the
> signaling/quiet bit).

I'm trying to understand the benefit in terms of reducing cost. Assuming we simply don't support WebAssembly on non-SSE2 machines I'm guessing we've got the cost of triaging sites that require WebAssembly. If the use of WebAssembly is correlated with WebGL (assuming games in the short term) then we've already got the issue of WebGL not being universally available.

Comment 12

[Mike Hommey [:glandium]]

Tests at compile time are not going to tell anything about runtime. The machines we build Firefox on have SSE2. The machines that run Firefox don't necessarily have it.

Now, if there's a runtime check that disables webassembly when SSE2 is not there, then you can build *the webassembly code* with SSE2 enabled. Assuming the code that does the runtime check is not in there.

The easy way to do that is to move the webassembly source declarations in a separate directory/moz.build with CXXFLAGS += CONFIG['SSE2_FLAGS'], and add DIRS += ['directory'] to js/src/moz.build.

(Note, js/src/moz.build should really split sources more generally, instead of having a huge list in one single file. That had a benefit back when the build system sucked more, but it hasn't been necessary for several years, now)

Note that SSE2_FLAGS doesn't contain -mfmpath=sse. It would probably be a good idea to add it there, but that might have unintended consequences on other code using those flags. So you may want a separate variable.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: build.patch

As discussed on irc, the files in wasm/ and some in jit/ can be compiled with these SSE2 flags; some MFBT headers are used by those files too.

I've started to experiment with splitting the moz.build files, however, I'm running into two issues:

- some files in jit/ should *not* get the SSE2 flags (only the one related to Ion should, and some files are related to Baseline, which doesn't necessarily need the SSE2 flags). How could I implement this within ion/moz.build?
- one of the MFBT functions, template<class From, class To> BitwiseCast(const From aFrom) -> To, located in mfbt/Casting.h, used to e.g. safely reinterpret a float32 as an int32, calls a variant BitwiseCast with the non SSE2 ABI (using the FP x87 stack again). I thought, since these functions are marked inline, and this is all in a header file, that passing the SSE2 flags for compiling the file including them would be enough to force usage of the SSE2 ABI. Do you have an idea of what is going here?

Comment 14

[Mike Hommey [:glandium]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #13)
> Created attachment 8786671 [details] [diff] [review]
> build.patch
> 
> As discussed on irc, the files in wasm/ and some in jit/ can be compiled
> with these SSE2 flags; some MFBT headers are used by those files too.
> 
> I've started to experiment with splitting the moz.build files, however, I'm
> running into two issues:
> 
> - some files in jit/ should *not* get the SSE2 flags (only the one related
> to Ion should, and some files are related to Baseline, which doesn't
> necessarily need the SSE2 flags). How could I implement this within
> ion/moz.build?

Why not move the baseline files?

> - one of the MFBT functions, template<class From, class To>
> BitwiseCast(const From aFrom) -> To, located in mfbt/Casting.h, used to e.g.
> safely reinterpret a float32 as an int32, calls a variant BitwiseCast with
> the non SSE2 ABI (using the FP x87 stack again). I thought, since these
> functions are marked inline, and this is all in a header file, that passing
> the SSE2 flags for compiling the file including them would be enough to
> force usage of the SSE2 ABI. Do you have an idea of what is going here?

Inline doesn't guarantee something is, in fact, inlined. The compiler can still decide not to inline. Try MOZ_ALWAYS_INLINE.

Comment 15

[Jeff Walden [:Waldo]]

MOZ_ALWAYS_INLINE doesn't necessarily guarantee inlining either, fwiw.  There's fundamentally no way to do what you want here short of inline assembly.  Of course, that might be the worst idea in the world, save for all the other ones we're from time to time currently trying.

Comment 16

[Mike Hommey [:glandium]]

MOZ_ALWAYS_INLINE should do the "right" thing on the architecture (singular) where using -mfpmath=sse -msse2 makes a difference.

But yes, theoretically, it's not guaranteed either. It all boils down to comment 4: if you want to fiddle with floats at the bit level, don't fiddle with floats ; fiddle with ints.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: arm-build.patch

I've taken down the other approach in bug 1248555 (fiddle with integers), but there were still some issues with the ARM simulator. As the ARM we're talking about is 32 bits, we're building the ARM simulator with x86 as the host, so the FP issues are leaking in the simulator itself.

I propose to change the build to include the right SSE flags **only for the simulator**. As it is used only on testing machines, we have full control and the SSE2 requirement for the users becomes orthogonal to the issue. The patch is a bit crude (pattern match Linux + ARM simulator build), but as there is only one platform building the ARM simulator on treeherder anyways, it seems fine.

I'd really like to have this before the next merge. Please let me know if I should add more configuration guards.

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Rephrasing the title.

I forgot to include an example of issue I had with the ARM simulator, in the previous comment: for instance, there's a WebAssembly test that ensures we don't use the x87 instructions for mul/div [1], and as the simulator performs these instructions under the host's processor (x86), it uses the x87 FPU and fails these tests.

[1] https://github.com/WebAssembly/spec/blob/master/ml-proto/test/float_misc.wast#L150-L155 (look for x87 in this file)

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8790379 [details] [diff] [review]
arm-build.patch

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

::: js/src/moz.build
@@ +483,5 @@
>              'jit/arm/Simulator-arm.cpp'
>          ]
> +        # Configuration used only for testing.
> +        if CONFIG['OS_ARCH'] == 'Linux':
> +            CFLAGS += [ '-msse2', '-mfpmath=sse' ]

There's only one C file declared in this moz.build, and I doubt it uses floats (it's only only used for --enable-vtune). IOW, I don't think you need to care about CFLAGS.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af1ba72b8809
Build the ARM simulator (on x86) with -msse2 -mfpmath=sse; r=glandium

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/af1ba72b8809