Closed
Bug 720169
Opened 12 years ago
Closed 12 years ago
IonMonkey: OSR entry executes loop test an extra time
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(2 files)
13.03 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
10.31 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
When doing OSR, the interpreter is at a JSOP_LOOPHEAD and reflects the state when taking the loop's backedge. OSR, however, merges its entry point into the loop's normal entry, which in a while or for-with-condition loop will execute the test an extra time before taking the backedge. When the loop test is effectful this leads to incorrect behavior. function foo() { var x = 0; var y = 0; while (x++ < 100) y++; print(y); } foo(); > js test.js 100 > js --ion test.js 99 (Aside: am3 in v8-crypto has such a loop, and has a correctness failure in jit-tests with --ion, so this may be involved there). The clean fix is to do OSR when the interpreter is beginning the loop test (which must happen every iteration), ideally with a JSOP_LOOPENTRY emitted at that point. Then OSR as it stands will work correctly, and future optimizations which hoist strengthened tests (e.g. bug 719541) will be simpler.
Assignee | ||
Updated•12 years ago
|
Assignee: general → bhackett1024
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #590518 -
Flags: review?(dvander)
Comment on attachment 590518 [details] [diff] [review] inbound patch (0c8e2e793851) Review of attachment 590518 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsopcode.tbl @@ +573,5 @@ > /* Push the implicit 'this' value for calls to the associated name. */ > OPDEF(JSOP_IMPLICITTHIS, 226, "implicitthis", "", 3, 0, 1, 0, JOF_ATOM) > + > +/* This opcode is the target of the entry jump for some loop. */ > +OPDEF(JSOP_LOOPENTRY, 227, "loopentry", NULL, 1, 0, 0, 0, JOF_BYTE) There's like 15 unused opcodes if you want to take one :)
Attachment #590518 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 3•12 years ago
|
||
Please don't mark this fixed when merging to mozilla-central, there is still some more work to do for this on the IonMonkey branch. https://hg.mozilla.org/integration/mozilla-inbound/rev/850ce7c81121
Comment 4•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/850ce7c81121
Target Milestone: --- → mozilla12
Comment 5•12 years ago
|
||
Does this still need to remain open?
Assignee | ||
Comment 6•12 years ago
|
||
Yes, I got distracted by threading stuff and forgot about this. Here is a patch against IM which passes jit-tests --ion -n
Attachment #590518 -
Attachment is obsolete: true
Attachment #592944 -
Flags: review?(dvander)
Assignee | ||
Updated•12 years ago
|
Attachment #590518 -
Attachment is obsolete: false
Comment on attachment 592944 [details] [diff] [review] patch (053bfa57297e) Review of attachment 592944 [details] [diff] [review]: ----------------------------------------------------------------- Cool, thanks for finding and fixing this!
Attachment #592944 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/afed24e840d8
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•