The default bug view has changed. See this FAQ.

IonMonkey: OSR entry executes loop test an extra time

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

unspecified
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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: general → bhackett1024
Created attachment 590518 [details] [diff] [review]
inbound patch (0c8e2e793851)
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+
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
https://hg.mozilla.org/mozilla-central/rev/850ce7c81121
Target Milestone: --- → mozilla12

Comment 5

5 years ago
Does this still need to remain open?
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
Attachment #590518 - Attachment is obsolete: true
Attachment #592944 - Flags: review?(dvander)
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+
https://hg.mozilla.org/projects/ionmonkey/rev/afed24e840d8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.