Closed
Bug 1207449
Opened 8 years ago
Closed 8 years ago
Differential Testing: Different output message involving Math.fround
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: gkw, Assigned: bbouvier)
References
Details
(Keywords: regression, testcase)
Attachments
(1 file)
2.90 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•8 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...
Assignee | ||
Comment 2•8 years ago
|
||
See comment above for explanation. This is the lazy, overly conservative approach.
Attachment #8665007 -
Flags: review?(hv1989)
Comment 3•8 years ago
|
||
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+
Assignee | ||
Comment 6•8 years ago
|
||
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•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dd8f99f562ae
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
![]() |
Reporter | |
Updated•8 years ago
|
Assignee: nobody → benj
You need to log in
before you can comment on or make changes to this bug.
Description
•