720169 - IonMonkey: OSR entry executes loop test an extra time

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

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 [Laid off!]]

Attachment: inbound patch (0c8e2e793851)

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

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 [Laid off!]]

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]]

https://hg.mozilla.org/mozilla-central/rev/850ce7c81121

Comment 5

[Paul Wright]

Does this still need to remain open?

Comment 6

[Brian Hackett [Laid off!]]

Attachment: 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] - inactive, e-mail if emergency]

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 [Laid off!]]

https://hg.mozilla.org/projects/ionmonkey/rev/afed24e840d8