Before working on SIMD Ion, there are a few operations that need to be fixed, to ensure the same behavior in interpreter and jit backend. Currently, there are three differences: - Out of bounds SIMD.loads (resp. stores) in asm.js load guard values (resp. do nothing). This will concern only asm.js vs interpreter at the moment, even when Ion will optimize SIMD, so doesn't need to get fixed right away. - Shift behavior, when the shift count is > 32. In the JITs, we have x86 SIMD shifts behavior (shift := min(shift, 32)); in the interpreter, C++ shifts behavior (shift := shift & 32). I'll go with x86 behavior, waiting for spec to settle a bit. - Errors in conversions from float32x4 to int32x4. This one is tricky (see details in [0]). Note that this bug is different from bug 1068028, which intends to fix behavior once it's actually specified. This bug will contain patches that make arbitrary choices, just to ensure ion/interpreter behaviors are the same. [0] https://github.com/johnmccutchan/ecmascript_simd/issues/99

Up to this patch, SIMD.int32x4.fromFloat32x4 would apply ecmascript ToInt32 coercion when converting operands (in particular imprecise float -> int32 conversions would behave as expected in ecmascript ToInt32), while the x86 JIT would yield the undefined integer pattern (0x80000000) on imprecise conversions (such as converting NaN / Infinity / Math.pow(2,32) to int32_t). Same thing applies for SIMD.int32x4.shifts, which behave like x86 with this patch. This might not be the final behavior, but it is a prerequisite for avoiding differential testing bugs between the interpreter and the JITs. Waldo, flagging you for review, especially for the float-to-int32 parts and the mfbt / templating magic. This is the least bad I could think of to ensure consistency across platforms (tested on an arm build, this seems to work), comments and other fixes would be greatly appreciated :)

Comment on attachment 8540202 [details] [diff] [review] SIMD.js: ensure consistent behavior in SIMD.int32x4.{fromFloat32x4,shifts} Review of attachment 8540202 [details] [diff] [review]: ----------------------------------------------------------------- Without a spec to consult, I really don't have a way to evaluate this patch as correct or incorrect on a number of different questions. And those questions are quite relevant to standardizing any particular algorithm, as a step you say you want to take after this lands. And there are places where I have no idea what exact choice is being made by the algorithm, and therefore whether that choice does or does not make any sense. I'm going to have to put a foot down: get specs to say exactly what should happened here, and I'll happily review that. But I don't have enough justification for the choices made (and none some choices that seem to have been made only accidentally, at best) to r+ this (or even f+ it) now. ::: js/src/builtin/SIMD.cpp @@ +17,5 @@ > #include "jsfriendapi.h" > > #include "builtin/TypedObject.h" > #include "js/Value.h" > +#include "mozilla/IntegerTypeTraits.h" This goes up above jsapi.h -- make check-style. @@ +565,3 @@ > }; > struct ShiftRightLogical { > + static inline int32_t apply(int32_t v, int32_t bits) { return bits > 31 ? 0 : uint32_t(v) >> bits; } Remove the redundant |inline| specifiers on all these, for readability. But, a worse problem. All the arguments are int32_t, which means you have to worry both about > 31, *and* about < 0. But you're not doing anything about the latter concern, so any of these can be trivially turned into C++ undefined behavior. (Which they were before, but visibly going halfway here gives a very wrong impression as to whether this is right or not.) Whatever spec is being written for this should define semantics in this case, and these functions should all implement them. @@ +774,5 @@ > > return StoreResult<Int32x4>(cx, args, result); > } > > +template<typename From, typename To> struct CanConvert {}; Seeing as it looks like CanConvert is always being used to access its value() static member function, don't define the class here -- just declare it. And, let's put empty lines between the specializations, and put the leading template bit on a separate line from the struct bit so the template bit and struct bits don't horribly blend together when reading. template<typename From, typename To> struct CanConvert; template<> struct CanConvert<int32_t, float> { static bool value(int32_t v) { .... } }; template<typename IntegerType> struct CanConvert<float, IntegerType> { static bool value(float v) { .... } }; @@ +777,5 @@ > > +template<typename From, typename To> struct CanConvert {}; > +template<> struct CanConvert<int32_t, float> { > + static bool value(int32_t v) { > + return true; This seems strange. int32_t -> float conversion is lossy for a bunch of numbers with magnitude greater than 2**24. What are the exact conversion semantics performed by this struct? Please add a comment nearby that explains exactly what they are. @@ +784,5 @@ > +template<typename IntegerType> struct CanConvert<float, IntegerType> { > + static bool value(float v) { > + return !mozilla::IsNaN(v) && > + v <= float(mozilla::MaxValue<IntegerType>::value) && > + v >= float(mozilla::MinValue<IntegerType>::value); NaN <= anything and the other way around are always false, so that check is unnecessary here (modulo compiler bugs). The conversion of the max/min values to float can easily be lossy. float(INT32_MAX) is float(2**31 - 1), but 2**31 - 1 can't be represented in an IEEE-754 single precision number -- and 2**32 as float probably (?, as I have no idea what semantics you want to implement here) shouldn't be convertible. Right? As for how to deal with this: I'm not honestly sure how to easily deal with this, other than to upcast both arguments to double or something. You really should go argue this out and get a behavior specified, keeping in mind that integer exact-representability-in-float checks are finicky and can't be implemented as conversions (unless you limit only to the exactly-representable range of numbers, e.g. |n| < 2**53 or so for doubles, but I have no idea whether that's desirable or not for this algorithm). @@ +803,5 @@ > Elem *val = TypedObjectMemory<Elem *>(args[0]); > RetElem result[Vret::lanes]; > + for (unsigned i = 0; i < Vret::lanes; i++) { > + result[i] = CanConvert<Elem, RetElem>::value(val[i]) ? ConvertScalar<RetElem>(val[i]) > + : 0x80000000; 0x80000000 is an unsigned value, so assigning this to a signed value doesn't make a whole lot of sense. Are you trying to get the most negative value, or what? If so, use mozilla/IntegerTypeTraits.h's MinValue<RetElem>::value. Basically, as with a bunch of this, I need a clearer statement of *what* semantics are being implemented here, to review this.

Thanks for the review, it indeed lacked a few things I hadn't thought about. While I really appreciate your comments here, as I was obviously missing cases that can appear to be obvious to somebody used to the process of finding such issues, I don't feel the public mocking on #jsapi (or #developers, can't remember, but that's quite not the point) with RyanVM was really constructive / useful / a nice thing to do to a co-worker. My impostor syndrom is already quite developed, no need to push it forward, thank you. Just asking for feedback here, as spec is (still) quite not stable. The best thing approaching a spec and that I can link here is sunfish's pull request on github [0], which defines a behavior that seems to solve all issues mentioned here. I'll set a reminder to ask for review in a month or so, if the Github PR gets landed till then. [0] https://github.com/johnmccutchan/ecmascript_simd/issues/72

And this part is for the float-to-int conversions, as defined by sunfish's pull request there [0]. It seems that the IsNaN check is actually needed, now that I've inverted the conditions. I put a few questions directly in the patch, look for the XXX marks. Namely, the int32-to-float conversions appear to be *almost* defined behavior: the C++ spec says (4.9.2) that if the input int32 value is in the range of float32 value (which is the case for all int32 inputs) but the input can't be represented exactly as a float32, then it is an implementation choice to use the next lower or higher representable value. Is it an issue that could show up in practice? [0] https://github.com/johnmccutchan/ecmascript_simd/pull/114

Tentatively asking for review here...

(In reply to Benjamin Bouvier [:bbouvier] from comment #4) > While I really appreciate your comments here, as I was obviously missing > cases that can appear to be obvious to somebody used to the process of > finding such issues, I don't feel the public mocking on #jsapi (or > #developers, can't remember, but that's quite not the point) with RyanVM was > really constructive / useful / a nice thing to do to a co-worker. Truly, I didn't mean to mock at all. I *thought* my comments were purely technical, and I mentioned the movie scene (only after it had been brought up) as something I was very much trying *not* to emulate -- an "anti-role model". Technical criticism is not a criticism of you, nor is it mockery of the patch. I don't think the patch can land as-is, and I think it needs a bit of adjustment, and more consistent approaches, before it can be used. But it's not wholly lacking in merit -- it just needs to be more consistent about what it's doing, and more clearly defining what those semantics are. > My impostor syndrom is already quite developed, no need to push it forward, > thank you. Hey, look, you're cool. You know a whole lot more than I do about the JITs, or about SIMD in general, for sure. My knowledge in those areas is mostly scattershot from little bits of work here and there, or from reviewing patches slightly over my head at the time and asking dumb questions, etc. :-) Everyone writes patches that get r- from time to time. Doesn't mean you're dumb or anything at all like that, just means that particular patch wasn't right for some reason. Nothing more.

Comment on attachment 8565371 [details] [diff] [review] shift-count-saturation.patch Review of attachment 8565371 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/SIMD.cpp @@ +641,5 @@ > struct WithW { > static inline T apply(int32_t lane, T scalar, T x) { return lane == 3 ? scalar : x; } > }; > struct ShiftLeft { > + static inline int32_t apply(int32_t v, int32_t bits) { return uint32_t(bits) >= 32 ? 0 : v << bits; } It occurs to me that in C++11, left-shift operations only have defined behavior *if* the operation, performed on mathematically exact values, produces a value that fits in the target type. That is, a call like ShiftLeft::apply(1 << 29, 3); has undefined behavior, because 1 << 32 doesn't fit in int32_t space. There's not an easy way to fix this, that I'm aware of. Add a comment pointing out that this is technically undefined behavior per C++11 [expr.shift]p2, and move on for now. @@ +649,3 @@ > }; > struct ShiftRightLogical { > + static inline int32_t apply(int32_t v, int32_t bits) { return uint32_t(bits) >= 32 ? 0 : uint32_t(v) >> bits; } All three of these methods are a bit large to be single-liners any more.

> ::: js/src/builtin/SIMD.cpp > @@ +641,5 @@ > > struct WithW { > > static inline T apply(int32_t lane, T scalar, T x) { return lane == 3 ? scalar : x; } > > }; > > struct ShiftLeft { > > + static inline int32_t apply(int32_t v, int32_t bits) { return uint32_t(bits) >= 32 ? 0 : v << bits; } > > It occurs to me that in C++11, left-shift operations only have defined > behavior *if* the operation, performed on mathematically exact values, > produces a value that fits in the target type. That is, a call like > > ShiftLeft::apply(1 << 29, 3); > > has undefined behavior, because 1 << 32 doesn't fit in int32_t space. > There's not an easy way to fix this, that I'm aware of. You can fix it by casting |v| to uint32_t, since shift overflow (though not shift *count* overflow) is well-defined for unsigned types.

Comment on attachment 8565372 [details] [diff] [review] throw-on-undefined-float-to-int32-conversions.patch Review of attachment 8565372 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/SIMD.cpp @@ +875,5 @@ > +}; > + > +// While int32 to float conversions can be lossy, these conversions have > +// defined behavior in C++, so we don't need to care about them here. > +// XXX is that correct? Sort of. Per [conv.fpint], """ If the value being converted is in the range of values that can be represented but the value cannot be represented exactly, it is an implementation-defined choice of either the next lower or higher representable value. """ If we pretend that implementation-defined maps to exactly what we want -- I guess that's round to nearest, even? -- then it's correct. That pretense is probably accurate, but I honestly don't know. And I don't know whether we would actually detect it if that pretense were incorrect in practice. Hopefully we have tests for a bunch such boundary values? If not, do some reading on IEEE-754 rounding modes and come up with a few. :-) This stuff's fascinating, it'll be fun! :-D @@ +888,5 @@ > + > +// Same argument as for int32 to float conversions. > +// XXX is that correct? > +template<> > +struct ThrowOnConvert<double, float> : public NeverThrow {}; This isn't accurate. C++11 [conv.double]: """ If the source value can be exactly represented in the destination type, the result of the conversion is that exact representation. If the source value is between two adjacent destination values, the result of the conversion is an implementation-defined choice of either of those values. Otherwise, the behavior is undefined. """ So converting Number.MAX_VALUE (the max finite IEEE-754 double value) to float is undefined. It's not in the float value set. And it's not between two adjacent destination values. Ergo, undefined. Note that Microsoft at least explicitly documents this behavior as undefined. https://msdn.microsoft.com/en-us/library/d3d6fhea.aspx @@ +898,5 @@ > +template<typename From, typename IntegerType> > +struct ThrowIfNotInRange > +{ > + static bool value(From v) { > + double d(v); static_assert(mozilla::IsIntegral<IntegerType>::value, "bad destination type"); or somesuch, at class level. ::: js/src/js.msg @@ +426,5 @@ > MSG_DEF(JSMSG_TYPEDOBJECT_BINARYARRAY_BAD_INDEX, 0, JSEXN_RANGEERR, "invalid or out-of-range index") > MSG_DEF(JSMSG_TYPEDOBJECT_HANDLE_UNATTACHED, 0, JSEXN_TYPEERR, "handle unattached") > MSG_DEF(JSMSG_TYPEDOBJECT_STRUCTTYPE_BAD_ARGS, 0, JSEXN_RANGEERR, "invalid field descriptor") > MSG_DEF(JSMSG_TYPEDOBJECT_TOO_BIG, 0, JSEXN_ERR, "Type is too large to allocate") > +MSG_DEF(JSMSG_SIMD_FAILED_FLOAT_TO_INT_CONVERSION, 0, JSEXN_RANGEERR, "Conversion from floating-point to integer failed.") The rest of patch context suggests don't capitalize, don't end in a period, is the convention. ::: js/src/tests/ecma_7/SIMD/int32x4fromfloat32x4.js @@ +25,5 @@ > + // INT32_MAX is the first value that's exactly representable as a float32 and > + // an int32. > + var d = float32x4(INT32_MAX - 127, INT32_MIN, -0, 0); > + var e = SIMD.int32x4.fromFloat32x4(d); > + assertEqX4(e, [INT32_MAX - 127, INT32_MIN, 0, 0]) Please check exact boundary values: INT32_MAX doesn't throw, INT32_MAX + Math.pow(2, -22) does throw, INT32_MIN doesn't throw, INT32_MIN - Math.pow(2, -21) does throw. ::: js/src/tests/ecma_7/SIMD/int32x4fromfloat64x2.js @@ +18,5 @@ > + > + var g = float64x2(INT32_MAX + 1, 0); > + assertThrowsInstanceOf(() => int32x4.fromFloat64x2(g), RangeError); > + > + var g = float64x2(INT32_MIN - 1, 0); Again worth the exact boundary-condition checks noted before, with INT32_* plus or minus the super-fractional values.

So I've had spent time with implementing a few ieee754 helpers, noting down a lot of examples and trying to understand things, and I got a few epiphanies regarding ieee754. Saying it was fun would be overzealous, but at least I've understood a few things better. It appears most implementation-defined choices are, (on my machine), round to nearest, tie to even. I'll need to try-run this, praying that all tier machines agree about this. For float64 to float32 conversions, I've did what seemed to make the most sense: NaN / + or - Infinity are fine; but if the double input can't fit into the float range, throw! Dan, does it make sense? Or should we let this implementation defined? For what it's worth: let double(s, e, f) be a double with s as a sign bit, e as the exponent and f as the fractional part. var biggestFloat = double(0, 1023 + 127, 0xfffffe0000000); Math.fround(biggestFloat) == biggestFloat; // true var nextToBiggestFloat = double(0, 1023 + 127, 0xfffffe0000001); Math.fround(nextToBiggestFloat) == biggestFloat; // true -- wat? and so on until var nextToBiggestFloat = double(0, 1023 + 127, 0xfffffefffffff); Math.fround(nextToBiggestFloat) == biggestFloat; // true -- wat? and then var nextToBiggestFloat = double(0, 1023 + 127, 0xffffff0000000); Math.fround(nextToBiggestFloat) == +Infinity; // true -- wat? That's a nice case of undefined behavior for conversions from float64 to float32, when the double input isn't in the float32 bounds... (or is it some rounding to nearest that I am not understanding here)

(In reply to Benjamin Bouvier [:bbouvier] from comment #12) > It appears most implementation-defined choices are, (on my machine), round > to nearest, tie to even. I'll need to try-run this, praying that all tier > machines agree about this. Yes. All machines should default to round-to-nearest, ties-to-even, and JS JITs should generally operate in that mode. > For float64 to float32 conversions, I've did what seemed to make the most > sense: NaN / + or - Infinity are fine; but if the double input can't fit > into the float range, throw! Dan, does it make sense? Or should we let this > implementation defined? Conversion from double to float is always well defined. On overflow, it goes to Infinity or -Infinity. > For what it's worth: let double(s, e, f) be a double with s as a sign bit, e > as the exponent and f as the fractional part. > > var biggestFloat = double(0, 1023 + 127, 0xfffffe0000000); > Math.fround(biggestFloat) == biggestFloat; // true Indeed. > var nextToBiggestFloat = double(0, 1023 + 127, 0xfffffe0000001); > Math.fround(nextToBiggestFloat) == biggestFloat; // true -- wat? IEEE 754-2008 7.4 says that overflow occurs: "[...] if the destination format’s largest finite number is exceeded in magnitude by what would have been the rounded floating-point result [...] were the exponent range unbounded." Your value here is only slightly greater than the largest finite number, and would have been rounded down were the exponent range unbounded. > and so on until > var nextToBiggestFloat = double(0, 1023 + 127, 0xfffffefffffff); > Math.fround(nextToBiggestFloat) == biggestFloat; // true -- wat? The same goes for all values up to this point. > and then > var nextToBiggestFloat = double(0, 1023 + 127, 0xffffff0000000); > Math.fround(nextToBiggestFloat) == +Infinity; // true -- wat? But here, the value would have been rounded up, and so the conversion overflows, and we get Infinity. I'd be surprised if there is relevant hardware which doesn't have this behavior.

Comment on attachment 8576843 [details] [diff] [review] throw.lossy.conversions.patch Review of attachment 8576843 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/SIMD.cpp @@ +879,5 @@ > +template<> > +struct ThrowOnConvert<float, double> : public NeverThrow {}; > + > +// Double to float conversion for inputs which aren't in the float range are > +// undefined behavior in C++. Otherwise, rounds to nearest, tie with even. The C++ standard is vague in this area. My interpretation is that the conversions should always be well defined on IEEE 754 representations, because such types can represent Infinity, and so all finite values are within their range. IEEE 754 for its part even specifies how the rounding is to be performed. But as long as this is just slow-path C++ code, I'm not super concerned about it. @@ +881,5 @@ > + > +// Double to float conversion for inputs which aren't in the float range are > +// undefined behavior in C++. Otherwise, rounds to nearest, tie with even. > +template<> > +struct ThrowOnConvert<double, float> Regardless of what C++ does though, JS shouldn't throw on this conversion. We should overflow to Infinity or -Infinity as needed. @@ +887,5 @@ > + static bool value(double d) { > + if (mozilla::IsNaN(d) || mozilla::IsInfinite(d)) > + return false; > + return d > double(std::numeric_limits<float>::max()) || > + d < -double(std::numeric_limits<float>::max()); This isn't quite right, because there is a range of double values which round down to FLT_MAX, as described in my previous post.

Same patch, with the float64 to float32 conversion never throwing, as requested by sunfish (this is indeed pretty well defined in ieee754, thanks Dan for explaining).

Comment on attachment 8578070 [details] [diff] [review] throw.on.lossy.patch Review of attachment 8578070 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/SIMD.cpp @@ +14,5 @@ > #include "builtin/SIMD.h" > > #include "mozilla/IntegerTypeTraits.h" > + > +#include <limits> Why do you need this? Looks like MinValue/MaxValue are all you need, and the #include above covers things. @@ +852,5 @@ > return StoreResult<Int32x4>(cx, args, result); > } > > +// This struct defines whether we should throw on convert, when trying to > +// convert from From to To. This happens whenever a C++ conversion would get "whether we should throw during a conversion attempt, when trying to convert a value of type From to the type To" @@ +853,5 @@ > } > > +// This struct defines whether we should throw on convert, when trying to > +// convert from From to To. This happens whenever a C++ conversion would get > +// undefined behavior (viz., be platform-dependent). s/get/have/ I will admit I'm not too big a fan of "viz." and would rather see just a simple "and perhaps" here. ::: js/src/js.msg @@ +444,5 @@ > MSG_DEF(JSMSG_TYPEDOBJECT_BINARYARRAY_BAD_INDEX, 0, JSEXN_RANGEERR, "invalid or out-of-range index") > MSG_DEF(JSMSG_TYPEDOBJECT_HANDLE_UNATTACHED, 0, JSEXN_TYPEERR, "handle unattached") > MSG_DEF(JSMSG_TYPEDOBJECT_STRUCTTYPE_BAD_ARGS, 0, JSEXN_RANGEERR, "invalid field descriptor") > MSG_DEF(JSMSG_TYPEDOBJECT_TOO_BIG, 0, JSEXN_ERR, "Type is too large to allocate") > +MSG_DEF(JSMSG_SIMD_FAILED_CONVERSION, 0, JSEXN_RANGEERR, "SIMD conversion (loss of precision)") Maybe "SIMD conversion loses precision" so it's a statement of what's wrong, instead of sort of two sentence fragments. ::: js/src/tests/ecma_7/SIMD/float32x4fromfloat64x2.js @@ +36,5 @@ > + mid = makeDouble(0, 1023, 0x0000030000000); > + assertEq((f2 + f3) / 2, mid); > + > + nextMid = makeDouble(0, 1023, 0x0000030000001); > + assertEq((f2 + f3) / 2, mid); You did this three lines above, and you haven't recomputed any of f2/f3/mid. Something's wrong here. ::: js/src/tests/ecma_7/SIMD/int32x4fromfloat32x4.js @@ +26,5 @@ > + assertEqX4(e, [lastFloat, 0, 0, 0]); > + > + // Test low boundaries > + assertEq(makeFloat(1, 158, 0), INT32_MIN); > + var d = float32x4(makeFloat(1, 127 + 31, 0), 0, 0, 0); Mildly nicer to use 127 + 31 for both of these. ::: js/src/tests/ecma_7/SIMD/shell.js @@ +1,1 @@ > +function makeFloat(sign, exp, bits) { bits is better named significand or mantissa. @@ +1,4 @@ > +function makeFloat(sign, exp, bits) { > + sign &= 0x1; > + exp &= 0xFF; > + bits &= 0x7FFFFF; Could you assert these bounds, perhaps, instead of masking? Masking could let garbage in -- better to be strict in what you accept (pace Postel). @@ +11,5 @@ > +} > + > +function makeDouble(sign, exp, bits) { > + sign &= 0x1; > + exp &= 0x7FF; Same comments. @@ +15,5 @@ > + exp &= 0x7FF; > + > + // Can't use bitwise operations on bits, as it might be a double > + assertEq(bits <= 0xfffffffffffff, true); > + var highBits = bits / Math.pow(2, 32) | 0; Add parentheses around the division? I never remember precedence for this stuff. @@ +21,5 @@ > + > + var i32 = new Int32Array(2); > + var f64 = new Float64Array(i32.buffer); > + > + i32[0] = lowBits; Blah, endianness. :-\

https://hg.mozilla.org/integration/mozilla-inbound/rev/15f18771c6ee https://hg.mozilla.org/integration/mozilla-inbound/rev/6c54c590a963 https://hg.mozilla.org/integration/mozilla-inbound/rev/f46048719af2

https://hg.mozilla.org/mozilla-central/rev/15f18771c6ee https://hg.mozilla.org/mozilla-central/rev/6c54c590a963 https://hg.mozilla.org/mozilla-central/rev/f46048719af2