Last Comment Bug 720169 - IonMonkey: OSR entry executes loop test an extra time
: IonMonkey: OSR entry executes loop test an extra time
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: mozilla12
Assigned To: Brian Hackett (:bhackett)
: Jason Orendorff [:jorendorff]
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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 User image 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)

> js test.js
> js --ion test.js

(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 User image Brian Hackett (:bhackett) 2012-01-21 15:28:37 PST
Created attachment 590518 [details] [diff] [review]
inbound patch (0c8e2e793851)
Comment 2 User image 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 User image 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.
Comment 4 User image Marco Bonardo [::mak] 2012-01-24 04:56:31 PST
Comment 5 User image Paul Wright 2012-01-30 17:42:33 PST
Does this still need to remain open?
Comment 6 User image 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 User image 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 User image Brian Hackett (:bhackett) 2012-01-31 15:55:48 PST

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