848679 - BaselineCompiler: Sync Ion and Baseline's treatment of where loops begin

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Founds this due to an orange caused by the second patch to bug 845873.  For whatever reason, it triggers this bug on tbpl builds for Fedora.  I'm not sure why because my changes should not be affecting any code that doesn't involve TypedArrays, which test jit-test/tests/jaeger/bug680842.js doesn't.

That's something that needs to be investigated.

That said, the issue is this:

Ion begins a for-loop's BasicBlock immediately after the source-note annotated instruction for the loop - not the actual LOOPHEAD.  For a loop with an empty conditional, we can get a sequence like this:

  POP
  NOP          <--- BasicBlock for loop body starts here.
  LOOPHEAD
  LOOPENTRY
  ...
  GOTO <LOOPHEAD>

Now, it's possible for Ion to bail out from the loop, and use the ResumePoint associated with the start of the loop - namely the NOP.

Baseline resumes execution at the NOP, hits the LOOPHEAD, and OSRs into Ion to re-execute the same loop iteration.  Ion bails to the NOP again, Baseline OSRs again, etc. etc.

Simple fix is just to have Ion capture the loop header's resume point AFTER the LOOPENTRY for these null-conditional for loops.

Comment 1

[Nicolas B. Pierron [:nbp]]

We had the a similar issue with the interpreter, and I had to add a trick for walking the instructions in ThunkToInterpreter.

Comment 2

[Kannan Vijayan [:djvj]]

Is it not possible to just have Ion skip the NOP+LOOPHEAD+LOOPENTRY before capturing the resume point for that block?

Comment 3

[Kannan Vijayan [:djvj]]

Ok so I know why this patch is triggering this bug, and it's not a big deal.

The patch changes Bailout/BoundsCheck behaviour in a reasonable way.  Before, the Ion would have been marked with bailoutExpected, but not invalidated - so the next OSR refuses to enter the ion code.  Now the script is invalidated and OSR recompiles it with Ion and tries to enter it again, leading to the infinite loop symptom we're seeing.

All is well.. just need to fix this bug.

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: Patch

Fixes the issue.

I suspect this problem still exists in the interpreter - since the bytecode-skipping implemented in ThunkToInterpreter doesn't skip past JSOP_NOP and JSOP_LOOPHEAD, which can occur with empty-condition loops.

Comment 5

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/projects/ionmonkey/rev/5e3ef592602b