538583 - JIT should emit LIR_j instead of LIR_jtbl if the latter only has one target

Status: RESOLVED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), enhancement)

Description

[Edwin Smith]

simple example comes from try/catch/finally:

function f() {
   try { }
   catch (e) { }
   finally { }
}

Generates a suboptimal sequence for OP_lookupswitch:

  120: jf ult2 -> B66
  121: jtbl ld8 [ B48 ]    <--- this could be LIR_j -> B48
  122: B66:

Also note the whole sequence could be replaced with a single branch and fall-through case, if the JIT notices that the default switch target is immediately next:

  120: jt ult2 -> B48     <-- invert the sense of the branch
  121: B66:               <-- only needed if other branches still land here

Comment 1

[Steven Johnson]

OP_lookupswitch already does this for count == 0.

Comment 2

[Edwin Smith]

exactly.  count==1 should generate a range check and a jump instead of a range check and a 1-entry jtbl.

Comment 3

[Steven Johnson]

Attachment: Patch

OP_lookupswitch the only user of LIR_jtbl, so the easy fix is there.

(Might be interesting to unroll switches of 2 or 3 too and see if that is faster...)

Comment 4

[Edwin Smith]

Comment on attachment 477633 [details] [diff] [review]
Patch

Looks pretty simple, and if tests pass then its probably fine.  But i'm also fighting a massive head cold so my judgement is cloudy.  Asking Bill for a second look.

As for unrolling jtbl into individual branches, that should be a backend-specific codegen decision, not in LIR (but still worth a try, its probably a win for low sized tables).

Comment 5

[Steven Johnson]

(In reply to comment #4)
> Looks pretty simple, and if tests pass then its probably fine.  But i'm also
> fighting a massive head cold so my judgement is cloudy.  Asking Bill for a
> second look.

Passes all tests (and it is being exercised, I checked). Doesn't move the needle noticeably in the perf tests I tried, but is arguably better code.

Comment 6

[William Maddox]

Comment on attachment 477633 [details] [diff] [review]
Patch

R+

Looks good to me.

Comment 7

[Steven Johnson]

pushed to TR as 5251:3381faf0c552