This should be straight-forward now, after the MConstant refactoring I did in bug 1246658 for int64. We can add MConstant::NewDouble (NewUncanonicalizedDouble?) and since we no longer use js::Value to store constants, it won't be canonicalized anywhere.

Taking.

Current wip. Many more tests are passing.

Now that we have much more tests passing, we would want to have them run in parallel, be able to run only once more easily, and not timeout when there are many to run. To accomplish this, I've made the following change: instead of spec.js importing a list of test, we now have each wast file have its equivalent JS runner file. The JS runner file imports spec.js and runs only this wast file. Because of how the jit_tests.py test script works, we can now just test a single wast file with the command jit_tests.py path/to/dist/bin/js float_exprs.wast and it will Just Work. This also documents the wast interpreter a bit better.

This simply runs the script created in the previous patch. (test cases *not* re-imported and updated yet, though)

This adds infra so that we can use the test mode for NaNs too. When the test mode is enabled, instead of canonicalizing and returning NaNs from wasm, we will create an object with the shape {nan_low, nan_high} containing two int32 containing the payload of the nan. For float32, nan_high will be set to -1 all the time, so that the test can just compare nan_low and nan_high without having to worry whether we're testing an f32 nan or a f64 nan. This adds some test infra in the JS files too, sanity checking that this works.

This tweaks operations to follow NaN semantics. I wish it were not an horror in which we monkey-patch plenty of MIR nodes for all the exceptions. But the only other way to do it I could think of would be to create new MIR nodes, and it just seemed more silly, so this is the least worse (?) solution, in my opinion. This makes us pass many more tests.

Comment on attachment 8758341 [details] [diff] [review] 3.test.infra.patch Review of attachment 8758341 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/WasmModule.cpp @@ +1304,5 @@ > } > > +template<typename T> > +static JSObject* > +CreateCustomNaNObjectOrValue(JSContext* cx, void* addr) Oops, the name dates from a time where this function would set args.rval() to the NaN object or the non-NaN number value. Locally renamed to CreateCustomNaNObject.

Comment on attachment 8758342 [details] [diff] [review] 4.nan.semantics.patch Review of attachment 8758342 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/WasmIonCompile.cpp @@ +505,5 @@ > + if (mustPreserveNaN(type)) { > + // Convert signaling NaN to quiet NaNs. > + MDefinition* one = constant(DoubleValue(1.0), type); > + lhs = mul(lhs, one, type, MMul::Normal); > + rhs = mul(rhs, one, type, MMul::Normal); Jakob on irc told that x - 0.0 also transforms signaling NaN into quiet NaNs and is cheaper to do (because 0.0 is only one xorps and sub is one cycle less than a mul, in the worst case, on some archs). Updated my patch locally.

Oops, I naively thought this would work on all the platforms if it'd work on x64: but it doesn't work on x86. So far my diagnostic is that on x86, SpecificNaN as defined in mfbt:: sets the quiet bit on signaling NaN, because it uses flds/fstps to return the value, and these instructions seem to transform sNaN into qNaN. On x64, we don't use those instructions, so we don't have this issue. Now, to find a workaround...

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57484#c11 says that we can't return a signaling NaN from a function without converting it into a quiet NaN. So there's more to do here...

Comment on attachment 8758341 [details] [diff] [review] 3.test.infra.patch Nice!

Comment on attachment 8758342 [details] [diff] [review] 4.nan.semantics.patch Review of attachment 8758342 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: js/src/asmjs/WasmBinaryIterator.h @@ -1285,5 @@ > - const float jsNaN = (float)JS::GenericNaN(); > - if (memcmp(Output ? f32 : &validateF32, &jsNaN, sizeof(*f32)) != 0) > - return notYetImplemented("NaN literals with custom payloads"); > - } > - With this code going away, it'd be nice to make readF32Const and readF64Const stylistically consistent with readI32Const and readI64Const -- rename validate{F32,F64} to unused and use && instead of an if. Or if you prefer, make the inverse changes to readI32Const and readI64Const :-). ::: js/src/asmjs/WasmIonCompile.cpp @@ +505,5 @@ > + if (mustPreserveNaN(type)) { > + // Convert signaling NaN to quiet NaNs. > + MDefinition* one = constant(DoubleValue(1.0), type); > + lhs = mul(lhs, one, type, MMul::Normal); > + rhs = mul(rhs, one, type, MMul::Normal); Looks good. This is obviously not pretty, but we can discuss what to do about the semantics elsewhere. ::: js/src/jit-test/tests/wasm/nan-semantics.js @@ +147,5 @@ > +test('f32', 'max', f32_snan_code, f32_zero, f32_qnan); > +test('f32', 'max', f32_zero, f32_snan_code, f32_qnan); > + > +test('f64', 'max', f64_snan_code, f64_zero, f64_qnan); > +test('f64', 'max', f64_zero, f64_snan_code, f64_qnan); Most of these are also covered in the spec tests. Having them here too isn't necessarily a problem though.

Thank you all for the reviews! (In reply to Dan Gohman [:sunfish] from comment #12) > ::: js/src/jit-test/tests/wasm/nan-semantics.js > @@ +147,5 @@ > > +test('f32', 'max', f32_snan_code, f32_zero, f32_qnan); > > +test('f32', 'max', f32_zero, f32_snan_code, f32_qnan); > > + > > +test('f64', 'max', f64_snan_code, f64_zero, f64_qnan); > > +test('f64', 'max', f64_zero, f64_snan_code, f64_qnan); > > Most of these are also covered in the spec tests. Having them here too isn't > necessarily a problem though. Yes indeed, but there are also very specific details that are tested here and not in the spec tests: in particular, this exercises more the GVN paths when one or both operands are constants. The spec tests don't test special cases when one of the operands is a constant. For what it's worth, not pushing yet, for two reasons: - the new testing scheme doesn't work on non x64: I thought it was neat to have test directives saying that a specific test would provoke an error, such that when we fix the error, the test would fail, and we could just remove the directive. But that doesn't quite work on x86 for some int64 tests, which are just skipped and don't fail although they are supposed to do so. - on x86, a signaling NaN that is returned (on the C++ stack) goes through fld / fstp instructions which set the quiet bit. I don't have yet good ideas for fixing this, unless materializing NaNs at the very last minute in MacroAssembler-x86.cpp, but that would mean having a new way to store floating point values as the raw uint32 / uint64 bits, and that would be pretty invasive.

Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/926f11001e01 run each WebAssembly spec test in its own script; r=mbx https://hg.mozilla.org/integration/mozilla-inbound/rev/7458396bf2c9 Import wast test JS runners; r=mbx

(landed only the testing changes -- the two custom NaN patches need a bit more work on x86/arm, but that's relatively low priority)

Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/56b9bf9665b1 Don't use the expected error directive in wasm/spec/; r=bustage

https://hg.mozilla.org/mozilla-central/rev/926f11001e01 https://hg.mozilla.org/mozilla-central/rev/7458396bf2c9 https://hg.mozilla.org/mozilla-central/rev/56b9bf9665b1

Rebased patch.

Carrying forward r+. Some tests don't pass anymore, because of failures in the baseline compiler, which landed before this got a chance to land.

Comment on attachment 8775104 [details] [diff] [review] 3.testing.patch Review of attachment 8775104 [details] [diff] [review]: ----------------------------------------------------------------- Just carrying forward r+.

(still WIP) some changes to MFBT.

x86 tests pass with this patch. The ARM simulator introduces another difficulty: the simulator itself returns floating-point values in sub rountines, so they need to be spot fixed, one by one.

This forces min/max to set the quiet bit for NaN inputs on x86/x64. Not pretty, but spec seems to say it has to do so. (Note on Ion we do the same thing; there were proposals to use different instructions that are cheaper in memory or in throughput, but I can't seem to find them back in my IRC backlog)

Nathan, when compiling with -msse2 -mfpmath=sse, the compiler passes floating point arguments on the regular stack using movss/movsd instructions (return values are still going through the x87 FP stack). This patch introduces two variants of MFBT functions that return values by outparam rather than by return, so we can preserve the signal bit in NaNs. I've added a small comment explaining the difference between the two functions: if you have a better wording in mind, please let me know.

ARM simulator changes only: - change a few functions to pass floating point results by out param rather than by return (the ARM simulator's host is x86). - inline an abs algorithm for vabs, to not use std::fabs (which returns by value). - canonicalize NaNs only in non-testing mode

Change a few functions to return by outparam rather than by value. With this patch and the one before + the dependent bug solved, all the tests pass on x64, x86 and ARM on my machine.

Re-asking review for the testing bits: it appears using an array of 2 int32 to form a double with those breaks the rules of strict aliasing, so we need to revert to using an uint64_t, bit fiddling and a real BitwiseCast (note the variant of BitwiseCast that takes an outparam is introduced in patch 6). I've also split the templated ReadCustomNaNObject method into two methods (one per type), because they became sufficiently different. The only changes to consider are in these two functions ReadCustom*NaNObject.

Comment on attachment 8776932 [details] [diff] [review] 3.testing.patch Review of attachment 8776932 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/WasmInstance.cpp @@ +427,5 @@ > } > > +template<typename T> > +static JSObject* > +CreateCustomNaNObject(JSContext* cx, void* addr) Can this be T* instead of void*? @@ +441,5 @@ > + if (!JS_DefineProperty(cx, obj, "nan_low", intVal, JSPROP_ENUMERATE)) > + return nullptr; > + > + intVal = Int32Value(IsSame<double, T>::value ? i32[1] : -1); > + if (!JS_DefineProperty(cx, obj, "nan_high", intVal, JSPROP_ENUMERATE)) Could you only JS_DefineProperty if IsSame<double, T> instead of defining a dummy property? It seems that would also match ReadCustomFloat32NaNObject which only stores a single property. ::: js/src/jit-test/tests/wasm/spec.js @@ +234,5 @@ > + > + // F32 NaN > + someNaN = wasmEvalText('(module (func (result f32) (f32.const -nan:0x123456)) (export "" 0))')(); > + i32[0] = someNaN.nan_low; > + i32[1] = someNaN.nan_high; I think you can remove nan_high.

Splitting the tests out of patch 4, so all the other patches can land without blocking on the dependent bug. Carrying forward r+ from sunfish.

Comment on attachment 8776915 [details] [diff] [review] 5.baseline-fixes.patch Review of attachment 8776915 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/WasmBaselineCompile.cpp @@ +3834,4 @@ > pushI32(r0); > } > > void Could you add a short TODO comment above this line that explains that we should avoid the sNaN->qNaN conversion in the following four methods if an operand is known to be a constant? Thanks.

Comment on attachment 8776918 [details] [diff] [review] 8-misc-changes.patch Review of attachment 8776918 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for tracking all these down!

Comment on attachment 8776916 [details] [diff] [review] 6-mfbt.patch Review of attachment 8776916 [details] [diff] [review]: ----------------------------------------------------------------- Gotta love x87. See comments below, and note that the Casting.h wordsmithing applies equally to the FloatingPoint.h comment. ::: mfbt/Casting.h @@ +16,5 @@ > > namespace mozilla { > > /** > + * Sets the out-param value of type |To| with the same underlying bit pattern of Nit: we use "outparam" everywhere...even if the hyphenated version might be more correct. Please change this instance and others. @@ +25,5 @@ > * platforms. > + * > + * There is also a variant that just returns the value. The out-param variant > + * preserves the signal bit in NaNs during floating-point conversions, while > + * the result variant may set the quiet bit for NaNs. My wordsmith sense says this can be improved, but I'm not sure what an improved wording would be. Perhaps something like: "There is also a variant that returns the value directly. On most architectures, the two variants should be identical, and you should prefer the version that returns values directly. On other architectures, however, returning NaN values directly silently turns signaling NaNs into quiet NaNs. On such architectures, the outparam version (along with appropriate compiler flags), should be used to avoid silent modification of data. This is more wordy, but it avoids mentioning the "signal bit" and "quiet bit", which is actually the same bit (!). I don't know whether it's worth dancing around the issue with "most architectures" and "some architectures", with the weirdness of the compiler flags bit thrown in. Maybe we should stop trying to be generic with: "There is also a variant that returns the value directly. In most cases, the two variants should be identical. However, in the specific case of x86 chips with SSE2 floating-point enabled in the compiler, the behavior differs: returning floating-point values directly is done through the x87 stack, and x87 loads and stores turn signaling NaNs into quiet NaNs...silently. Returning floating-point values via outparam, however, is done entirely within the SSE registers, which has semantics-preserving behavior you would expect. If preserving the distinction between signaling NaNs and quiet NaNs is important to you, you should use the outparam version. In all other cases, you should use the direct return version." (I don't remember whether it's x87 loads or stores or both that strips the signaling behavior; I trust you can modify the comment appropriately.) WDYT? @@ +30,4 @@ > */ > template<typename To, typename From> > +inline void > +BitwiseCast(const From aFrom, To* result) Nit: aResult. ::: mfbt/FloatingPoint.h @@ +255,3 @@ > template<typename T> > +static MOZ_ALWAYS_INLINE void > +SpecificNaN(int signbit, typename FloatingPoint<T>::Bits significand, T* result) NB: |result| here is fine, as the code in this file hasn't been Gecko-namified.

Thank you all for the reviews. (In reply to Nathan Froyd [:froydnj] from comment #33) > This is more wordy, but it avoids mentioning the "signal bit" and "quiet > bit", which is actually the same bit (!). They are indeed the same bit; I've been looking too closely at this issue and now in my head the signal bit (resp. quiet bit) means "this specific bit set to signal" (resp. "to not signal"). > "There is also a variant that returns the value directly. In most cases, > the two variants should be identical. However, in the specific case of x86 > chips with SSE2 floating-point enabled in the compiler, the behavior > differs: returning floating-point values directly is done through the x87 > stack, and x87 loads and stores turn signaling NaNs into quiet > NaNs...silently. Returning floating-point values via outparam, however, is > done entirely within the SSE registers, which has semantics-preserving > behavior you would expect. > > If preserving the distinction between signaling NaNs and quiet NaNs is > important to you, you should use the outparam version. In all other cases, > you should use the direct return version." > > (I don't remember whether it's x87 loads or stores or both that strips the > signaling behavior; I trust you can modify the comment appropriately.) > > WDYT? I prefer the specific wording that mentions x86 and SSE2 too, thanks. Indeed the x87 loads/stores (fld/fstp) both change the signaling behavior, so keeping them both in the comment. I am going to move the "with SSE2 floating-point enabled in the compiler" to the sentence talking specifically about outparams, since this is where this actually matters. Returning values on x86 uses the x87 stack, whether SSE2 floating point support is enabled or not in the compiler. Passing floating-point values on x86 depends whether you've enabled SSE2 or not. Thank you for the wording.

Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/89a131cc0bfd Introduce variants of SpecificNaN / BitwiseCast that preserve the signaling NaN bit; r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/08619103cd0b Return floating-point values by out-param rather than by value; r=sunfish https://hg.mozilla.org/integration/mozilla-inbound/rev/b2411c64362e Baldr: Add support for testing NaN custom payloads; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f310c9b166 Baldr: implement custom NaN payload semantics; r=sunfish

Still leave-open: tests can land once the dependent bug is fixed.

Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf6aed414cb Changes to the ARM simulator; r=sunfish https://hg.mozilla.org/integration/mozilla-inbound/rev/75a607ece852 Baseline fixes for handling custom NaN payloads; r=lth

(In reply to Benjamin Bouvier [:bbouvier] from comment #34) > Thank you all for the reviews. > > (In reply to Nathan Froyd [:froydnj] from comment #33) > > This is more wordy, but it avoids mentioning the "signal bit" and "quiet > > bit", which is actually the same bit (!). > > They are indeed the same bit; I've been looking too closely at this issue > and now in my head the signal bit (resp. quiet bit) means "this specific bit > set to signal" (resp. "to not signal"). Indeed! Looking at other literature that discusses this, I think "quiet bit" or "signal bit" are used somewhat interchangeably, but for people just reading through, it would be good to attempt to avoid confusion. > Thank you for the wording. Thanks for clarifying and seeing this through.

https://hg.mozilla.org/mozilla-central/rev/89a131cc0bfd https://hg.mozilla.org/mozilla-central/rev/08619103cd0b https://hg.mozilla.org/mozilla-central/rev/b2411c64362e https://hg.mozilla.org/mozilla-central/rev/e5f310c9b166 https://hg.mozilla.org/mozilla-central/rev/fdf6aed414cb https://hg.mozilla.org/mozilla-central/rev/75a607ece852

Another (WIP) approach that wouldn't require the dependency on the build system changes.

Tests (carrying forward r+). Only change was to update to the new import rules (the function name can't be empty).

This changes strategies a bit: instead of building with -msse -mfpmath=sse (which we can't do, because we still want to support no-sse users, sigh), we can just fiddle with integers everywhere from parsing down to code generation on x86. This is a bit invasive, but proves efficient. Some functions which took double/float parameters now have a variant that use uint64_t/uint32_t *pointer* parameters (if they were not pointer types, the compiler would use implicit type coercions). Changes leak into: - x64 because of code shared between x86 and x64 :( - ARM because the simulator runs under x86 :( It's unfortunate, but this doesn't make us depend on the build changes, so we can have it now in time for the wasm RFC/RC version, and we can enable many more tests from the spec suite.

Comment on attachment 8790375 [details] [diff] [review] int-fp-values.patch Review of attachment 8790375 [details] [diff] [review]: ----------------------------------------------------------------- Would mozilla::Variant make this code any better?

(In reply to Nathan Froyd [:froydnj] from comment #43) > Comment on attachment 8790375 [details] [diff] [review] > int-fp-values.patch > > Review of attachment 8790375 [details] [diff] [review]: > ----------------------------------------------------------------- > > Would mozilla::Variant make this code any better? There's only one tagged union (AstConst), and it seems overkill to use a mozilla::Variant there. Did you have other uses in mind, or do you strongly think it'd be better to have a mozilla::Variant in AstConst?

Better patch addressing a small issue shown by try (visitMToDouble can create LDouble too).

Comment on attachment 8791170 [details] [diff] [review] fp-int.patch Review of attachment 8791170 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +1498,5 @@ > static MConstant* New(TempAllocator::Fallible alloc, const Value& v, > CompilerConstraintList* constraints = nullptr); > static MConstant* NewFloat32(TempAllocator& alloc, double d); > + static MConstant* NewRawFloat32(TempAllocator& alloc, uint32_t* bits); > + static MConstant* NewRawDouble(TempAllocator& alloc, uint64_t* bits); I like the motivation to avoid the implicit casts which will surely save us all a lot of frustration. A pointer suggests outparam, though, so the interface looks a bit confusing until you understand the motivation. What if instead you added a struct RawDouble { explicit RawDouble(double); static RawDouble fromBits(uint64_t); } ? The explicit ctor could do the BitwiseCast internally which could remove a lot of the BitwiseCasts added in this patch.

Sounds good, I'll look into that. Current status: after bug 1302723, there is another issue on win64 only (sigh) that I need to investigate (another rounding that should not happen).

We actually need to delete RawDouble(uint32_t) to trigger a compiler error (explicit is not enough). At the limit, probably ::fromBits static method shouldn't allow non-integer types, so maybe we should actually have 2 classes for each of (double,float): RawDoubleFromFP(double) { } RawDoubleFromFP(uint64_t) = delete; RawDoubleFromFP(int64_t) = delete; RawDoubleFromBits(double) = delete; RawDoubleFromBits(uint64_t) { } RawDoubleFromBits(int64_t) // not really needed But the static method already makes the intent quite explicit, so we could also keep it that way.

Comment on attachment 8791953 [details] [diff] [review] fp-int.patch Review of attachment 8791953 [details] [diff] [review]: ----------------------------------------------------------------- Looking pretty good, thanks for all this work! I have a few further questions/comments, mostly relating to asking: what is the interface/invariant which tells us when to use Raw<T> vs. T? Suggestion below: ::: js/src/asmjs/WasmAST.h @@ +243,5 @@ > + // bit when passing the value as arguments. > + union Stored { > + Val val_; > + jit::RawFloat32 f32_; > + jit::RawDouble f64_; Since this seems like a permeating wasm problem, could we push the RawFloat/Double into Val so that way we're doing this regularly everywhere? Moreover, should we simply more-or-less ban use of 'float' and 'double' in asmjs/? ::: js/src/asmjs/WasmBinary.h @@ +515,5 @@ > } > MOZ_MUST_USE bool writeFixedU32(uint32_t i) { > return write<uint32_t>(i); > } > + MOZ_MUST_USE bool writeFixedU64(uint64_t i) { Could you instead remove this and change writeFixedF32/F64 to take RawFloat/RawDouble? ::: js/src/jit/IonTypes.h @@ +760,5 @@ > > +// Two structs which can encode any float32/double as raw bits values, > +// including NaN values for which the quiet/signal bit must be preserved. > + > +struct RawDouble Since these types are primarily needed for wasm, could you put them in WasmTypes.h? WasmTypes.h has a number of other structs that are used in MIR nodes (like CalleeDesc). This helps the jit/ reader understand why they are having to care about float vs. wasm::Raw<float>. Maybe also good to expand on the comment to describe the situation and also how we canonicalize at wasm/JS boundaries b/c of NaN-boxing. Also, I think you could define this as a template Raw<T> that uses a supporting: template <class T> RawIntType {}; template <> RawIntType<float> { typedef uint32_t T; }; template <> RawIntType<double> { typedef uint64_t T; }; to pick the integer type. That would remove the need for RawSelector. @@ +780,5 @@ > + r.value_ = bits; > + return r; > + } > + uint64_t bits() const { return value_; } > + double fp() const { return mozilla::BitwiseCast<double>(value_); } Does "fp" stand for "floating point"? Perhaps "f64" and "f32" as method names would be a bit more clear? ::: js/src/jit/MIR.h @@ +1498,5 @@ > static MConstant* New(TempAllocator::Fallible alloc, const Value& v, > CompilerConstraintList* constraints = nullptr); > static MConstant* NewFloat32(TempAllocator& alloc, double d); > + static MConstant* NewRawFloat32(TempAllocator& alloc, RawFloat32 bits); > + static MConstant* NewRawDouble(TempAllocator& alloc, RawDouble bits); Since I think the types are not inter-convertible, perhaps you could make these just be New() overloads? This would avoid the apparent redundancy in "MConstant::NewRawFloat32(..., RawFloat32(...))". ::: js/src/jit/arm/Simulator-arm.cpp @@ +3691,5 @@ > if (instr->szValue() == 0x1) { > + union { > + double f64; > + uint64_t u64; > + } u; Could you use RawDouble (and RawFloat below) instead of this union? ::: js/src/jit/x64/MacroAssembler-x64.h @@ +866,5 @@ > void loadConstantFloat32(float f, FloatRegister dest); > void loadConstantSimd128Int(const SimdConstant& v, FloatRegister dest); > void loadConstantSimd128Float(const SimdConstant& v, FloatRegister dest); > > + // Variants that preserve NaN patterns. Should they be variants or should the originals be removed? ::: js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp @@ +85,5 @@ > + RawDouble raw; > + if (MConstant* cst = ins->mir()) > + raw = cst->toRawDouble(); > + else > + raw = RawDouble(ins->getDouble()); Why this instead of having LDouble store a RawDouble? ::: js/src/jit/x86/MacroAssembler-x86-inl.h @@ +183,5 @@ > > void > MacroAssembler::addConstantDouble(double d, FloatRegister dest) > { > + Double* dbl = getDouble(RawDouble(d)); Should the RawDouble-ness get propagated outward into addConstantDouble's signature?

Thanks for the review! I like the invariant of trying to maximize the use of the Raw<T> structs under wasm/, so I've done that. This also "forced" to implement the Right Thing for the baseline compiler and for the binaryTo*Text transforms, so I've done that too as a bonus. I've got a new version that addresses most comments, except for a few: (In reply to Luke Wagner [:luke] from comment #49) > @@ +780,5 @@ > > + r.value_ = bits; > > + return r; > > + } > > + uint64_t bits() const { return value_; } > > + double fp() const { return mozilla::BitwiseCast<double>(value_); } > > Does "fp" stand for "floating point"? Perhaps "f64" and "f32" as method > names would be a bit more clear? Yes, that was meaning to mean "floating point" indeed; naming f32 or f64 limits the usage in templates (and would require explicit subclassing instead of a simple "using RawFloat32 = Raw<float>;"), so I'd like to keep a unique name for both types. Maybe "real" (as in the set of real numbers, in opposition to the integer representation), but even that sounds a bit unclear. I've let it as is (fp) in the current patch, but pretty happy to change the name if you have a great idea. > ::: js/src/jit/arm/Simulator-arm.cpp > @@ +3691,5 @@ > > if (instr->szValue() == 0x1) { > > + union { > > + double f64; > > + uint64_t u64; > > + } u; > > Could you use RawDouble (and RawFloat below) instead of this union? Good question! I've tried it, and I actually could not, because of the x87 stack :) The method Raw<T>::fp() returns a plain T and can't return a const& T (because the return value is a temporary), so it uses the x87 stack (yes, even when compiling with SSE2), and at some point we'd have to call Raw<T>::fp(). Plus the code would not even be prettier: double d; get_double_value_from_register(&d); RawDouble raw(d); raw = RawDouble::fromBits(raw.bits() & mask); double d_value = raw.fp(); // (fwiw, here's the call causing the signal->quiet conversion) > ::: js/src/jit/x64/MacroAssembler-x64.h > @@ +866,5 @@ > > void loadConstantFloat32(float f, FloatRegister dest); > > void loadConstantSimd128Int(const SimdConstant& v, FloatRegister dest); > > void loadConstantSimd128Float(const SimdConstant& v, FloatRegister dest); > > > > + // Variants that preserve NaN patterns. > > Should they be variants or should the originals be removed? Many callers on the JITs (esp. Ion) don't care about Raw<T>, so I've kept the plain T variants that avoid infecting all the callers with wasm::Raw<T>. > ::: js/src/jit/x86/MacroAssembler-x86-inl.h > @@ +183,5 @@ > > > > void > > MacroAssembler::addConstantDouble(double d, FloatRegister dest) > > { > > + Double* dbl = getDouble(RawDouble(d)); > > Should the RawDouble-ness get propagated outward into addConstantDouble's > signature? For the same reason as above, I don't think so.

And now with WasmTextUtils.

Comment on attachment 8793312 [details] [diff] [review] rawint.patch Review of attachment 8793312 [details] [diff] [review]: ----------------------------------------------------------------- Great, thanks again for all the hard work tracking all these down! ::: js/src/asmjs/WasmBinary.h @@ +54,5 @@ > + MOZ_IMPLICIT Raw(uint16_t) = delete; > + MOZ_IMPLICIT Raw(int32_t) = delete; > + MOZ_IMPLICIT Raw(uint32_t) = delete; > + MOZ_IMPLICIT Raw(int64_t) = delete; > + MOZ_IMPLICIT Raw(uint64_t) = delete; I think you can achieve this with template <class U> Raw(U) = delete; I did a few local tests and it seemed to have the exact desired effect. @@ +63,5 @@ > + T fp() const { return mozilla::BitwiseCast<T>(value_); } > +}; > + > +using RawDouble = Raw<double>; > +using RawFloat32 = Raw<float>; Since now we're in wasm-land, how about RawF64/RawF32 to avoid the current syntactic asymmetry? Also shorter :) @@ +785,5 @@ > MOZ_MUST_USE bool readFixedU32(uint32_t* u) { > return read<uint32_t>(u); > } > + MOZ_MUST_USE bool readFixedU64(uint64_t* u) { > + return read<uint64_t>(u); I think this method is dead now. ::: js/src/asmjs/WasmIonCompile.cpp @@ +2031,3 @@ > break; > case ValType::F64: > + result = f.constant(DoubleValue(value.f64().fp()), mirType); Shouldn't these two pass the Raw<T> instead? ::: js/src/asmjs/WasmTextToBinary.cpp @@ +1586,5 @@ > + > + value = (isNegated ? Traits::kSignBit : 0) | Traits::kExponentBits | value; > + return new (c.lifo) AstConst(Val(Raw<Float>::fromBits(value))); > + > +error: nit: two space indent for labels ::: js/src/asmjs/WasmTextUtils.h @@ +28,5 @@ > +MOZ_MUST_USE bool > +RenderInBase10(StringBuffer& sb, uint64_t num); > + > +MOZ_MUST_USE bool > +RenderInBase16(StringBuffer& sb, uint64_t num); IIUC, this doesn't need to be in the header and the caller inside WasmTextUtils.cpp could call RenderInBase<16> directly.

Good catch on the globals! Actually, there was just a few bits missing so that we can test them...

Comment on attachment 8793392 [details] [diff] [review] globals.patch Review of attachment 8793392 [details] [diff] [review]: ----------------------------------------------------------------- Great, thanks! I think we should collect our (Read|Create)(I64|CustomNaN)Object functions into one place. Even though these are currently in Wasm(Instance|Module), the "right" place feels like WasmJS.h since it's responsible for the JS binding.

Good idea.

Comment on attachment 8793695 [details] [diff] [review] codemotion.patch Review of attachment 8793695 [details] [diff] [review]: ----------------------------------------------------------------- thanks! ::: js/src/asmjs/WasmJS.h @@ +57,5 @@ > +// T=float or T=double. > + > +template<typename T> > +JSObject* > +CreateCustomNaNObject(JSContext* cx, T* addr); uber-nit: can you make the order of declarations match the order of definitions in WasmJS.cpp?

Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1984eea180d6 Use integers as a low-level representation of floating-point values; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/cef1dab54ded Addendum: support uncanonicalized NaNs in globals too; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/68ae4c840079 Move {Read,Create}{I64|CustomNaN}Object into WasmJS.h; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/e504c8940148 Tests only; r=sunfish

Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5e73a7285b Unbreak the build; r=me

Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/30a689e356c9 Block a few slow tests under CGC; r=jonco

https://hg.mozilla.org/mozilla-central/rev/1984eea180d6 https://hg.mozilla.org/mozilla-central/rev/cef1dab54ded https://hg.mozilla.org/mozilla-central/rev/68ae4c840079 https://hg.mozilla.org/mozilla-central/rev/e504c8940148 https://hg.mozilla.org/mozilla-central/rev/4f5e73a7285b https://hg.mozilla.org/mozilla-central/rev/30a689e356c9

https://hg.mozilla.org/mozilla-central/rev/1984eea180d6 https://hg.mozilla.org/mozilla-central/rev/cef1dab54ded https://hg.mozilla.org/mozilla-central/rev/68ae4c840079 https://hg.mozilla.org/mozilla-central/rev/e504c8940148 https://hg.mozilla.org/mozilla-central/rev/4f5e73a7285b https://hg.mozilla.org/mozilla-central/rev/30a689e356c9