Differential Testing: Different output message involving Math.fround

RESOLVED FIXED in Firefox 44



JavaScript Engine: JIT
2 years ago
2 years ago


(Reporter: gkw, Assigned: bbouvier)


(Blocks: 2 bugs, {regression, testcase})

regression, testcase
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)



(1 attachment)



2 years ago
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

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

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?
Flags: needinfo?(hv1989)

Comment 1

2 years ago
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

2 years ago
Created attachment 8665007 [details] [diff] [review]

See comment above for explanation. This is the lazy, overly conservative approach.
Attachment #8665007 - Flags: review?(hv1989)
Comment on attachment 8665007 [details] [diff] [review]

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.
Attachment #8665007 - Flags: review?(hv1989) → review+
Flags: needinfo?(hv1989)

Comment 6

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


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.
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44


2 years ago
Assignee: nobody → benj
You need to log in before you can comment on or make changes to this bug.