Implement "one-shot" interceptors.

Implement "one-shot" interceptors.
Committed: https://code.google.com/p/dart/source/detail?r=17558

[ahe, 2013-01-23 20:06:56]

What is a one-shot interceptor?

[sra1, 2013-01-23 21:21:30]

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2100: for
(Selector selector in backend.oneShotInterceptors) {
These need to be sorted to have stable output.

It would be useful to have a canonical sort order for selectors.  If you think
this is a separate CL, add an issue to the tracker to implement and use the
order here and make Issue 2388 blocking on it.

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2109:
arguments.add(new js.VariableUse('receiver'));
also: js.use('receiver')

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2127: new
js.Return(
also: js.return_(x)

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:490:
SourceString name = Elements.operatorNameToIdentifier(selector.name);
Can typed selectors get here?
There may be many typed selectors with the same name.
Adding the type to the name would cause the names to be more consistent.

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2165: String toString()
=> 'one short interceptor on $selector';
short -> shot

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1451: * This phase
changes all interceptors that are used only once with
/changes/replaces/

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1472: // the
interceptor is not needed.
Does this lead to unreferenced one-shots?

[kasperl, 2013-01-24 07:19:14]

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2098: void
emitOneShotInterceptors(CodeBuffer buffer) {
How difficult would it be to build these one shot interceptors in SSA form
instead? The reason I ask is that it would be nice for us to get to a point
where we could unfold the call to getInterceptor like this:

  function foo$oneshot(receiver, x) {
    if (typeof receiver == "number")
      return $.JSNumber.prototype.foo(receiver, x);
    else 
      return $.ObjectInterceptor.prototype.foo(receiver, x);
  }

and then let the builder decide if it wants to inline the calls to two different
calls to foo (based on the code size implications, etc.).

[ngeoffray, 2013-01-24 08:39:15]

Thanks all. PTAL.

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2098: void
emitOneShotInterceptors(CodeBuffer buffer) {
On 2013/01/24 07:19:14, kasperl wrote:
> How difficult would it be to build these one shot interceptors in SSA form
> instead? The reason I ask is that it would be nice for us to get to a point
> where we could unfold the call to getInterceptor like this:
> 
>   function foo$oneshot(receiver, x) {
>     if (typeof receiver == "number")
>       return $.JSNumber.prototype.foo(receiver, x);
>     else 
>       return $.ObjectInterceptor.prototype.foo(receiver, x);
>   }
> 
> and then let the builder decide if it wants to inline the calls to two
different
> calls to foo (based on the code size implications, etc.). 

As discussed, I agree it will be pretty neat to rely on the SSA to do further
optimizations. I'll leave that to another CL. Filed
https://code.google.com/p/dart/issues/detail?id=8089 to not forget about it.

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2100: for
(Selector selector in backend.oneShotInterceptors) {
On 2013/01/23 21:21:30, sra1 wrote:
> These need to be sorted to have stable output.
> 
> It would be useful to have a canonical sort order for selectors.  If you think
> this is a separate CL, add an issue to the tracker to implement and use the
> order here and make Issue 2388 blocking on it.

Issue filed: https://code.google.com/p/dart/issues/detail?id=8088.

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2109:
arguments.add(new js.VariableUse('receiver'));
On 2013/01/23 21:21:30, sra1 wrote:
> also: js.use('receiver')

Done.

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2127: new
js.Return(
On 2013/01/23 21:21:30, sra1 wrote:
> also: js.return_(x)

Done.

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:490:
SourceString name = Elements.operatorNameToIdentifier(selector.name);
On 2013/01/23 21:21:30, sra1 wrote:
> Can typed selectors get here?
> There may be many typed selectors with the same name.
> Adding the type to the name would cause the names to be more consistent.

I believe typed selector can come here, but I'd prefer not too for now. I added
a TODO to filter them out.

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2165: String toString()
=> 'one short interceptor on $selector';
On 2013/01/23 21:21:30, sra1 wrote:
> short -> shot

Done.

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1451: * This phase
changes all interceptors that are used only once with
On 2013/01/23 21:21:30, sra1 wrote:
> /changes/replaces/

Done.

https://codereview.chromium.org/12033056/diff/10/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1472: // the
interceptor is not needed.
On 2013/01/23 21:21:30, sra1 wrote:
> Does this lead to unreferenced one-shots?

One-shots have side effects, so there's nothing we can do about unreferenced
ones.

[kasperl, 2013-01-24 09:03:51]

LGTM.

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:674: final
Set<Selector> oneShotInterceptors;
One option would be to use a SelectorMap and make that support ordered
iteration. Let's think about that.

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:31: final
Map<Selector, String> oneShotInterceptorNames;
If you used a SelectorMap in the emitter, it might make sense to store the names
there? Not quite sure. It feels a little bit weird that we need to mappings for
the one shot interceptors.

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1634: if
(node.isInterceptorCall) {
Use ?: to avoid having an uninitialized valueType variable.

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart (right):

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:57: if
(user.bothNotInLoopOrInSameLoop(input)) {
With this name, it seems weird to call this one of the arguments. Maybe change
this to user.hasSameLoopHeaderAs(input)?

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1167: bool
bothNotInLoopOrInSameLoop(HInstruction other) {
hasSameLoopHeaderAs

[ngeoffray, 2013-01-24 10:41:30]

Thanks Kasper.

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:31: final
Map<Selector, String> oneShotInterceptorNames;
On 2013/01/24 09:03:51, kasperl wrote:
> If you used a SelectorMap in the emitter, it might make sense to store the
names
> there? Not quite sure. It feels a little bit weird that we need to mappings
for
> the one shot interceptors.

As discussed, let's keep it here for now to make it consistent of how codegen
and emitter use the namer, and think about removing state in the namer.

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1634: if
(node.isInterceptorCall) {
On 2013/01/24 09:03:51, kasperl wrote:
> Use ?: to avoid having an uninitialized valueType variable.

Done.

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart (right):

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:57: if
(user.bothNotInLoopOrInSameLoop(input)) {
On 2013/01/24 09:03:51, kasperl wrote:
> With this name, it seems weird to call this one of the arguments. Maybe change
> this to user.hasSameLoopHeaderAs(input)?

Done.

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12033056/diff/5002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1167: bool
bothNotInLoopOrInSameLoop(HInstruction other) {
On 2013/01/24 09:03:51, kasperl wrote:
> hasSameLoopHeaderAs

Done.