Closed Bug 1207449 Opened 10 years ago Closed 10 years ago

Differential Testing: Different output message involving Math.fround

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: gkw, Assigned: bbouvier)

References

Details

(Keywords: regression, testcase)

Attachments

(1 file)

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?
Flags: needinfo?(hv1989)
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...
Attached patch bug1207449.patchSplinter 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] 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.
Attachment #8665007 - Flags: review?(hv1989) → review+
*too
Flags: needinfo?(hv1989)
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee: nobody → benj
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: