Closed Bug 609129 Opened 9 years ago Closed 9 years ago

TM: fix emitIf(), re-enable implicit guard optimization

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: fixed-in-nanojit,fixed-in-tracemonkey,fixed-in-tamarin)

Attachments

(2 files)

Attached file reproducer
Discussion in bug 607856 has identified potential problems in checkTraceEnd() and emitIf().  dvander said in bug 607856 comment 16:

> checkTraceEnd() forcefully resets cx->regs->sp to the original |sp|. That's
> super scary, since it's assuming that everything above the entry sp is dead
> (which is not necessarily true, like here).
>
> I vote for figuring out why checkTraceEnd() is doing shenanigans.. but we could
> also change emitIf() to not remove guards if the op is JSOP_IFNE?

Attached file is a minimal reproducer of the problem.  It loop infinitely if the implicit guard removal from bug 600127 is enabled.
Assignee: nnethercote → general
Status: ASSIGNED → NEW
Here's the relevant code:

  ------------------------------ # JSOP_POP
  ------------------------------ # JSOP_GOTO
  ------------------------------ # JSOP_GOTO
  ------------------------------ # JSOP_GETLOCAL
      sti.sp sp[0] = ATOM_TO_STRING(atom)/*0xf6d00020*/
  ------------------------------ # JSOP_AND
      mLengthAndFlags2 = ldi.str ATOM_TO_STRING(atom)/*0xf6d00020*/[0]
      length2 = rshui mLengthAndFlags2, immi1/*4*/
      eqi1 = eqi length2, strict/*0*/
      xf1: xf eqi1 -> pc=0x8f14e42 imacpc=(nil) sp+8 rp+0 (GuardID=006)
  ------------------------------ # JSOP_IFNE
**    xt1: xt eqi1 -> pc=0x8f14e48 imacpc=(nil) sp+8 rp+0 (GuardID=007)
      x1: x  -> pc=0x8f14e0e imacpc=(nil) sp+0 rp+0 (GuardID=008)

When implicit guard optimization is on, xt1 is removed, causing the infinite loop.
The problem is that xt1 and x1 have different destinations -- xt1 leaves this fragment, but x1 returns to the top of this fragment.

(The original version of this program in bug 607856 was slightly different, and there was also a stack store that was removed when the guard was removed.)
The attached patch re-enables implicit guard optimization and appears to fix
the emitIf() problems.  Both the test attached to this bug and the one
attached to bug 607856 now work.  Also, the website that was hanging in bug
607856 no longer does.

LIR generated for the test from bug 607856 (which is a tougher test, because
it includes the stack store) with current tip:

  ------------------------------ # JSOP_IFEQ
      mLengthAndFlags = ldi.str ATOM_TO_STRING(atom)/*0xf6c00020*/[0]
      immi1 = immi 4
      length = rshui mLengthAndFlags, immi1/*4*/
      eqi1 = eqi length, strict/*0*/ 
      xf1: xf eqi1 -> pc=0x9df1e20 imacpc=(nil) sp+8 rp+0 (GuardID=002)
  ------------------------------ # JSOP_GOTO
  ------------------------------ # JSOP_GOTO
  ------------------------------ # JSOP_GETLOCAL
      sti.sp sp[0] = ATOM_TO_STRING(atom)/*0xf6c00020*/
  ------------------------------ # JSOP_AND
  ------------------------------ # JSOP_IFNE
      xt1: xt eqi1 -> pc=0x9df1e40 imacpc=(nil) sp+8 rp+0 (GuardID=004)
      x1: x  -> pc=0x9df1e0a imacpc=(nil) sp+0 rp+0 (GuardID=005)

LIR generated with the patch:

  ------------------------------ # JSOP_IFEQ
      mLengthAndFlags = ldi.str ATOM_TO_STRING(atom)/*0xf6c00020*/[0]
      immi1 = immi 4
      length = rshui mLengthAndFlags, immi1/*4*/
      eqi1 = eqi length, strict/*0*/
      xf1: xf eqi1 -> pc=0x9c53e20 imacpc=(nil) sp+8 rp+0 (GuardID=002)
  ------------------------------ # JSOP_GOTO
  ------------------------------ # JSOP_GOTO
  ------------------------------ # JSOP_GETLOCAL
      sti.sp sp[0] = ATOM_TO_STRING(atom)/*0xf6c00020*/
  ------------------------------ # JSOP_AND
  ------------------------------ # JSOP_IFNE
      x1: x  -> pc=0x9c53e40 imacpc=(nil) sp+8 rp+0 (GuardID=003)

Lovely!  xt1 is gone but x1 has 'sp+8' so the stack store remains.

The patch itself just pushes the "cond-ification" of the relevant LIR
instructions outside emitIf(), avoiding the possibility of the
inconsistencies that were happening when it was done inside emitIf().  The
ridiculous w.eqi0(w.eqi0(...)) sequences are necessary because LIR lacks a
LIR_nei instruction (bug 573416 is open for this), and emitIf() doesn't work
if you invert 'cond' and 'x' in tandem.  (This latter fact confused me for a
while.)

Now, as for whether checkTraceEnd() is correct, I don't know, but it doesn't
seem to be hurting this test.  As it happens, 'pendingLoop' is now false on
the case of interest, so checkTraceEnd() calls endLoop() instead.
Assignee: general → nnethercote
Status: NEW → ASSIGNED
Attachment #487781 - Flags: review?(dvander)
Comment on attachment 487781 [details] [diff] [review]
patch (against TM 56625:64e76d8afd09)

Well, this patch doesn't fix the fact that any of this is scary ;) But I can't think of a test case that would break. If pendingLoop is false, we want to always exit the trace, and in that case, checkTraceEnd() won't hijack cx->regs->sp. If pendingLoop is true, and the guard is eliminated, the loop edge is an iloop anyway.
Attachment #487781 - Flags: review?(dvander) → review+
Summary: TM: checkTraceEnd() is super scary; emitIf() may be too → TM: fix emitIf(), re-enable implicit guard optimization
nnethercote http://hg.mozilla.org/tamarin-redux/rev/fa8c300aa1a8
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit,fixed-in-tracemonkey,fixed-in-tamarin
http://hg.mozilla.org/mozilla-central/rev/635225cbcaea
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 609308
You need to log in before you can comment on or make changes to this bug.