724631 - Check OSR condition inline

Status: VERIFIED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P3)

Description

[Edwin Smith]

Existing jit code looks like this when it checks the OSR condition at function entry:

  if (adjustFrame(...))
     goto loop
  rest-of-prolog

Its faster if we inline the fast path, like this:

  if (*(&exec->current_osr)) {
    adjustFrame(...)
    goto loop
  }
  rest-of-prolog

Comment 1

[Edwin Smith]

Attachment: Inline the fast path for the osr branch check

Comment 2

[William Maddox]

Comment on attachment 594774 [details] [diff] [review]
Inline the fast path for the osr branch check

Review of attachment 594774 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  I'm curious how much speedup we get from this.  Seems like an easy win in any case.

Comment 3

[Edwin Smith]

In big alchemy apps with OSR enabled, I noticed adjustFrame() using a bunch of time on its fast path.  If you have a function with a loop that gets hot via the loop-check, *and* that function is called a lot, then we benefit from streamlining this check.

We could likely do better by compiling two versions of the function (normal-entry and loop-entry) and discarding the loop-entry function once all its stack frames die.  But this patch is simple and effective for now.  progress not perfection.

Dir: v8.5/optimized/
  crypto                     4233.0  4229.7  4574.0  4513.0     8.1     6.7 ++
  deltablue                   914.0   913.0   959.0   957.7     4.9     4.9 + 
  earley-boyer                781.0   778.3   815.0   812.7     4.4     4.4 + 
  raytrace                   4135.0  4131.0  4244.0  4228.3     2.6     2.4 + 
  regexp                       74.6    74.5    78.1    77.9     4.7     4.7 + 
  richards                   1420.0  1419.7  1499.0  1497.7     5.6     5.5 ++
  splay                      4087.0  3920.3  4310.0  4056.7     5.5     3.5

Comment 4

[Tamarin Bot]

changeset: 7188:a169e8a8d172
user:      Edwin Smith <[e-mail]>
summary:   Bug 724631 - Check OSR condition inline (r=wmaddox+)

http://hg.mozilla.org/tamarin-redux/rev/a169e8a8d172

Comment 5

[Brent Baker]

Changeset 7188:a169e8a8d172 has introduced many failures when running the acceptance suite with -osr=17. The following assertion is triggered when run with a debug build:

There are about 800+ test abcs that cause this assertion, but this is the abc that I used to verify:
as3/Definitions/Function/EmptyFunctionBody.abc

Assertion failure: LIR type error (emitStart): arg 1 of 'eqi' is 'ldq' which has type quad (expected int): 0 (../nanojit/LIR.cpp:4074)
...

#4  0x00000001001964e1 in nanojit::ValidateWriter::typeCheckArgs (this=0x10121a6e8, op=nanojit::LIR_eqi, nArgs=2, formals=0x7fff5fbfd6d0, args=0x7fff5fbfd6c0) at ../nanojit/LIR.cpp:4069
#5  0x0000000100196f8d in nanojit::ValidateWriter::ins2 (this=0x10121a6e8, op=nanojit::LIR_eqi, a=0x10121d048, b=0x101211268) at ../nanojit/LIR.cpp:4450
#6  0x00000001000c1619 in nanojit::LirWriter::ins2 (this=0x10121a710, v=nanojit::LIR_eqi, a=0x10121d048, b=0x101211268) at LIR.h:1741
#7  0x00000001000c188e in nanojit::LirWriter::ins2ImmI (this=0x10121a710, v=nanojit::LIR_eqi, oprnd1=0x10121d048, imm=0) at LIR.h:1829
#8  0x00000001000c18bd in nanojit::LirWriter::insEqI_0 (this=0x10121a710, oprnd1=0x10121d048) at LIR.h:1818
#9  0x00000001000c1c24 in avmplus::LirHelper::eqi0 (this=0x7fff5fbfdd58, ptr=0x10121d048) at LirHelper-inlines.h:144
#10 0x00000001000ac1b7 in avmplus::CodegenLIR::branchToLabel (this=0x7fff5fbfdd50, op=nanojit::LIR_jf, cond=0x10121d048, label=@0x10121c1a8) at ../core/CodegenLIR.cpp:8151
#11 0x00000001000aeb29 in avmplus::CodegenLIR::emitOsrBranch (this=0x7fff5fbfdd50) at ../core/CodegenLIR.cpp:1985
#12 0x00000001000bf01f in avmplus::CodegenLIR::writePrologue (this=0x7fff5fbfdd50, state=0x10103e410, pc=0x1011b6c63 "!???f\016$", driver=0x7fff5fbfdb40) at ../core/CodegenLIR.cpp:1935
....

Comment 6

[Tamarin Bot]

changeset: 7191:718b6e929bd8
user:      Edwin Smith <[e-mail]>
summary:   Bug 724631 - Check OSR condition inline (2nd fix)

http://hg.mozilla.org/tamarin-redux/rev/718b6e929bd8

Comment 7

[Brent Baker]

Verified fixed in change 7191:718b6e929bd8