Last Comment Bug 720169 - IonMonkey: OSR entry executes loop test an extra time
: IonMonkey: OSR entry executes loop test an extra time
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on:
Blocks: 719541
  Show dependency treegraph
 
Reported: 2012-01-21 14:10 PST by Brian Hackett (:bhackett)
Modified: 2012-01-31 15:55 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
inbound patch (0c8e2e793851) (13.03 KB, patch)
2012-01-21 15:28 PST, Brian Hackett (:bhackett)
dvander: review+
Details | Diff | Splinter Review
patch (053bfa57297e) (10.31 KB, patch)
2012-01-30 17:55 PST, Brian Hackett (:bhackett)
dvander: review+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2012-01-21 14:10:58 PST
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.
Comment 1 Brian Hackett (:bhackett) 2012-01-21 15:28:37 PST
Created attachment 590518 [details] [diff] [review]
inbound patch (0c8e2e793851)
Comment 2 David Anderson [:dvander] 2012-01-23 12:56:01 PST
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 :)
Comment 3 Brian Hackett (:bhackett) 2012-01-23 13:52:24 PST
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 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-01-24 04:56:31 PST
https://hg.mozilla.org/mozilla-central/rev/850ce7c81121
Comment 5 Paul Wright 2012-01-30 17:42:33 PST
Does this still need to remain open?
Comment 6 Brian Hackett (:bhackett) 2012-01-30 17:55:17 PST
Created attachment 592944 [details] [diff] [review]
patch (053bfa57297e)

Yes, I got distracted by threading stuff and forgot about this.  Here is a patch against IM which passes jit-tests --ion -n
Comment 7 David Anderson [:dvander] 2012-01-30 19:40:12 PST
Comment on attachment 592944 [details] [diff] [review]
patch (053bfa57297e)

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

Cool, thanks for finding and fixing this!
Comment 8 Brian Hackett (:bhackett) 2012-01-31 15:55:48 PST
https://hg.mozilla.org/projects/ionmonkey/rev/afed24e840d8

Note You need to log in before you can comment on or make changes to this bug.