1326027 - wasm: Remove the RawF32/RawF64 in favor of ye olde float/double

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wip.patch

Now that we compile with SSE2 flags on intel archs, this should be enough to not use the x87 fp stack anymore.

Attached is a patch that removes the two structs and replaces them by float/double. A try build [0] showed that:
1. Win x86 tests are busted
2. ARM simulator build is busted

Since this Just Works for Linux x86, I assume the build has to be tweaked to use sse2 even for the ARM simulator. For win x86, some investigation needs to be done too; some recent try logs seem to show that the sse2 flag is explicitly passed, soooooooo.

[0] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca243c27b7d0744df4d103c7ebc02d8d62083183

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: remove-from-mfbt.patch

(second part for mfbt changes)

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

After an afternoon of tinkering with this issue and try-building there: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ae0dff3c15bc08efb7f93ab99e91840e6d24009&selectedJob=33501703, it appears that:

1. the NaN tests don't pass with a linux x86 debug non-optimized build on my machine, even if explicitly compiling with -msse -msse2 -mfpmath=sse, because the x87 FP stack is still used for internal calls to functions returning a FP value
s. (the normal stack is used for passing arguments) An instance of this is e.g. a call to mozilla::BitwiseCast<float> in ParseNaNLiteral<float>, or js::wasm::Decoder::readFixedF32() in WasmIonCompile.cpp (the former we could just inline, the latter we can't).

2. the NaN tests *do* pass with a linux x86 debug optimized build on my machine (!). The code generated is different with -O3 and doesn't use the x87 FP stack in this case (probably because of aggressive inlining that removes the need to pass the result back through the x87). So this is why the try build is all green for linux x86 debug on treeherder: this is actually a debug + optimized build.

3. *I think* the shell builds generated with autospider don't use the same config as the entire browser, namely they don't include the SSE2 flags. Evidence of this is that the linux x86 debug optimized build passed the NaN tests, but the ARM simulator debug optimized build on the same platform did not. I think some differences could be observable with respect to canonical NaN handling; fortunately this is not very common, but this *could* be an issue. Filing a new bug for that.

Anyhow, not even spending some time on the Win32 issues, considering that the NaN behavior is working nicely now (despite the awkwardness of the code), I am not sure spending more time on trying to fix this is valuable, so closing as WONTFIX. If somebody wants to take over, please be my guest.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Re-reading the removed comment in the mfbt patch, I think the way to go would be to:
- enable sse2 everywhere we can (i.e. change autospider)
- change functions that return FP values to have an out-param instead
- ditch the mfbt removal patch
I'll take a look at this tomorrow.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.remove-raw.patch

Last try build went muuuuuch greener. I was actually close yesterday...

This:
- removes the RawF32 (resp. RawF64) and replaces instances by uses of float (resp. double).
- makes some methods return const T& instead of T, to force usage of the SSE instructions and normal stack. (this is consistent behavior on gcc, clang and win's cl)
- makes some methods take a T outparam when the T value has to be computed, to force usage of SSE instructions and normal stack too.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.build-shells-sse.patch

The shell builds on treeherder were not compiled with SSE2 support (on x86). Now that we have dropped support for non-sse2, and since it's enabled in browser builds even for linux32 now [1], we should enable these in shell builds too, for more consistency.

[1] http://searchfox.org/mozilla-central/source/build/unix/mozconfig.linux32#7

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8822427 [details] [diff] [review]
1.remove-raw.patch

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

Ah, ok, I think I get it: the -m flags can't change the calling convention on x86 -- it's frozen -- and while parameters are specified to always be passed by the stack, float return values are defined to always use ST(0).  Thus our engine-wide invariant is that float/double return values in C++ code that runs x86 needs to use an outparam or const&.  Thanks for investigating!

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +899,2 @@
>          uint32_t slot() const { MOZ_ASSERT(kind_ > MemLast && kind_ <= LocalLast); return slot_; }
>          uint32_t offs() const { MOZ_ASSERT(isMem()); return offs_; }

nit: I'd align slots() and offs() too.

Comment 7

[Lars T Hansen [:lth]]

Benjamin: Please return float and double here, not references into the stack.  The stack may in principle change arbitrarily after these are called but before the values are used, even if it does not, currently.

+        const float&  f32val() const { MOZ_ASSERT(kind_ == ConstF32); return f32val_; }
+        const double& f64val() const { MOZ_ASSERT(kind_ == ConstF64); return f64val_; }

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c07ff4ab630
Remove RawF32 and RawF64; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/f99c84213375
include SSE2 flags in autospider; r=sfink

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c07ff4ab630
https://hg.mozilla.org/mozilla-central/rev/f99c84213375