517888 - Optimize load when calling a function in a declared slot

Status: VERIFIED FIXED

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

Description

[Edwin Smith]

we're missing this optimization:

class A {
  var f
}
function test(a:A) {
 a.f()
}

We can early bind to the slot A.f, even if we have to call the value as a closure.  Currently we're doing the slow path which has to look up A.f at runtime in addition to calling the result as a closure.

Comment 1

[Edwin Smith]

Attachment: early bind slot load in OP_callprop (call to closure in slot)

Some refactoring included, to make this maintainable:

* loading from slot is relegated to loadFromSlot() helper that doesn't update FrameState, so it can be used as an intermediate step.  emitGetSlot() now calls loadFromSlot in the obvious way.

* loadAtomRep factored to take Traits* as an argument, when the input value isn't something sitting in a variable slot tracked in FrameState

* loadAtomRep cleaned up to switch builtin type instead of testing each type one at a time.  removed some redundant constant folding (that is already handled by nanojit::ExprFilter)

* callprop_late calls callprop_late_slot when we dynamically discover the same situation.

Comment 2

[Tom Harwood]

Comment on attachment 401854 [details] [diff] [review]
early bind slot load in OP_callprop (call to closure in slot)

I see loadAtomRep() has lost the constant micro-opt for boolean types.  Overlooked or not relevant?

Comment 3

[Edwin Smith]

it is redundant with constant folding performed later in the pipeline by nanojit::ExprFilter

Comment 4

[Rick Reitmaier]

Comment on attachment 401854 [details] [diff] [review]
early bind slot load in OP_callprop (call to closure in slot)

(1) Does newly added getBinding() call at 3570 introduce a new path for an exception to get fired (e.g.kAmbiguousBindingError)

(2) may need to add 'break;'  to each case for some brain-dead compilers.

Comment 5

[Edwin Smith]

(In reply to comment #4)
> (From update of attachment 401854 [details] [diff] [review])
> (1) Does newly added getBinding() call at 3570 introduce a new path for an
> exception to get fired (e.g.kAmbiguousBindingError)

Nope, this code is copied right from Toplevel::getBinding which also can throw a kAmbiguousBindingError

I cringe at "copied" so I'm looking for a better way to factor the code to avoid the copying.  will repost if this code changes substantially as a result.

Comment 6

[Steven Johnson]

(In reply to comment #4)
> (2) may need to add 'break;'  to each case for some brain-dead compilers.

I've yet to see a compiler require that particular stupidity. Have you seen one?

Comment 7

[Rick Reitmaier]

(In reply to comment #6)
> (In reply to comment #4)
> > (2) may need to add 'break;'  to each case for some brain-dead compilers.
> 
> I've yet to see a compiler require that particular stupidity. Have you seen
> one?

I thought we saw it (or something similar) on the sparc.  But lets go without it, until proven otherwise.

Comment 8

[Edwin Smith]

I've bumped into problems when the last case of a switch falls through, but this code doesn't do that, all cases return.  and i've never had problems with that pattern iirc.

Comment 9

[Edwin Smith]

this patch needs to be rebased, it currently depends on the OP_callproperty cleanup code which is stalled.

Comment 10

[Edwin Smith]

Attachment: Rebased and updated

previous patch was sufficiently changed to deserve re-review

* no longer based on (abandoned) callprop cleanup patch
* includes use of slightly optimized op_call

Comment 11

[Edwin Smith]

pushed

Comment 12

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.