958381 - Differential Testing: Different output message involving Math.fround

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

function f(x) {
    return (x != x) != Math.fround(x)
}
var m = [];
m.push(f(0));
m.push(f(0));
print(m);

$ ./js --fuzzing-safe --ion-parallel-compile=off 49.js
false,false

$./js --fuzzing-safe --ion-parallel-compile=off --ion-eager 49.js
false,true

Tested on js 64-bit debug threadsafe shell on m-c changeset effa95701c83 on Mac.

My configure flags are:

CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/8621bdc40841
user:        Benjamin Bouvier
date:        Wed Sep 11 02:10:17 2013 -0700
summary:     Bug 900257: Inline Math.fround in IonMonkey; r=sstangl

Benjamin, is bug 900257 a likely regressor?

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Investigating. Oddly enough, can't reproduce with --ion-parallel-compile=on.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

The last comparison gets wrong in IonMonkey. It becomes a comparison between Values (talking about compareType here), with the LHS being a Value tagged as a Boolean (x != x) and the RHS a value tagged as a Double (the float Math.fround(x) gets converted to a double, as expected). As the comparison occurs between Values with a different tag, it obviously fails.

When modifying the code to ensure that the RHS is a Double value instead (s/Math.fround(x)/-.5+x+.5/), it just uses a comparison with the type Compare_DoubleMaybeCoerceLHS (or RHS, can't remember) and thus it works.

Very nice test case by the way!

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Patch + test

If a MCompare doesn't have float32 producers as inputs, it just becomes (or stays) a double comparison. But the MCompare::trySpecializeFloat32 function doesn't apply any coercion even if one is needed, in the fallback path. With this patch, if any of the sides may need a coercion while the other element is a float32, then we set the correct type of comparison during type inferring. By now, only the CompareDouble type is specialized for float32 in MCompare::trySpecializeFloat32, so any CompareDoubleMaybeCoerce*HS will imply that we fall back to a double comparison. Problem solved.

We could enhance this by also optimizing float32 comparison with elements that need a coercion (a kind of inlined CompareFloatMaybeCoerce*HS), but this would be less open to optimizations than the double equivalent, as we can't ensure that we're not losing any precision, without any ranges (that we don't have at this point). That can be done for numerical constants, and booleans for instance, like in this test case. Not sure it's worth it though.

Drive by clean: the compareType_ check at the beginning of trySpecializeFloat32 cannot be reached, as the MCompare either comes from AsmJS in which case the float32 specialization phase never happens, or the compareType_ is set to Float32 in the same function afterwards.

Comment 5

[Sean Stangl [:sstangl]]

Comment on attachment 8359327 [details] [diff] [review]
Patch + test

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

Looks fine to me. Thanks for the explanation, also!

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87d628b0e1a8

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/87d628b0e1a8