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

Status: RESOLVED FIXED

Description

[Gary Kwong [:gkw] [:nth10sd]]

function f(x) {
    return (Math.fround((x ? 0 : x) | Math.max(x, x)));
}
for (var j = 0; j < 2; ++j) {
    print(f((function(){}) - 4294967297 ? 1 : 1 | 0 && 4294967297));
}

$ ./js-dbg-64-dm-nsprBuild-darwin-a1ccea59e254 --fuzzing-safe --ion-offthread-compile=off --baseline-eager testcase.js
1
1

$ ./js-dbg-64-dm-nsprBuild-darwin-a1ccea59e254 --fuzzing-safe --ion-offthread-compile=off --ion-eager testcase.js
1
0

Tested this on m-c rev a1ccea59e254.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r a1ccea59e254

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2db399cd414f
parent:      257780:0760af2a400f
user:        Hannes Verschore
date:        Fri Aug 14 11:46:28 2015 +0200
summary:     Bug 1193112: IonMonkey - Let the float32 optimization work with Float32, r=bbouvier

Hannes, is bug 1193112 a likely regressor?

Comment 1

[Benjamin Bouvier [:bbouvier] (away until Jan 24th)]

A MMathMinMax is generated for the Math.max(x, x), in which x is a phi whose both inputs are filtertypesets. Unfortunately, these two filtertypesets inputs also are phis.

The float32-mark-producers works like this:
- mark all phis as potential float32 producers
- reduce this choice based on phi's inputs (for each input, mark as a potential float32 producer if it is a unanalyzed phi or if it can produce a float32), until a fixed point is reached.

Unfortunately, the analysis gets nuts because the MFilterTypeSet's inputs are phis; so when calling MFilterTypeSet::canProduceFloat32() during the phi analysis, this function returns true at this point. Later, when all phis are analyzed, it will return false, but it's already too late: the phi has been marked as a float32 producer. Then later, the MMinMax is specialized to a float32 because both phis are (incorrectly) marked as float32 producers.

I see two ways of fixing this:
- either making the analysis more generic, so that we can not only have MPhis in the temporary marking lists, but also MFilterTypeSet (and maybe other instructions?).
- or be very conservative and make MFilterTypeSet::canProduceFloat32() (resp. canConsumeFloat32) return false if the input (resp. any use) is a phi.

What do you think? Second way is trivial, but it might impact performance...

Comment 2

[Benjamin Bouvier [:bbouvier] (away until Jan 24th)]

Created attachment 8665007 [details] [diff] [review]
bug1207449.patch

See comment above for explanation. This is the lazy, overly conservative approach.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8665007 [details] [diff] [review]
bug1207449.patch

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

Sadly r+. Don't expect to many regression, though it is a sore point. Just looks like I opened a can of worms.

Comment 4

[Hannes Verschore [:h4writer]]

*too

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd8f99f562ae

Comment 6

[Benjamin Bouvier [:bbouvier] (away until Jan 24th)]

For history: I've thought about another case that could be problematic, which is the following:

FilterTypeSet(FilterTypeSet(Phi))

but this patch is so conservative that this actually isn't a problem (the inner FilterTypeSet won't be marked as a producer nor a consumer).

Opened bug 1208447 as a potential, non-trivial follow-up.

Comment 7

[Carsten Book [:Tomcat]]

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