- Move length getter and setter interceptors to the new interceptor scheme.

- Move length getter and setter interceptors to the new interceptor scheme.
- Remove obsolete HInvokeInterceptor instruction
- Transform an intercepted length getter on string/array to a HFieldGet.
Committed: https://code.google.com/p/dart/source/detail?r=15192

[ngeoffray, 2012-11-20 15:09:09]

I filed bug http://code.google.com/p/dart/issues/detail?id=6829 to make sure we
port back the old optimizations we were doing on interceptors.

[karlklose, 2012-11-20 16:40:35]

LGTM.

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

https://codereview.chromium.org/11412105/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1324: // We know it's a
selector call if it follows the interceptor
How about 
// We know it is an interceptor call, if it has two more
// arguments than parameters expected from the selector: the
// receiver of the interceptor call and the receiver.
return inputs -1 == selector.argumentCount;

https://codereview.chromium.org/11412105/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1327: return
inputs.length - 1 != selector.argumentCount;
Trailing whitespace.

[Johnni Winther, 2012-11-21 08:20:29]

lgtm

[karlklose, 2012-11-21 08:34:55]

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

https://codereview.chromium.org/11412105/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1324: // We know it's a
selector call if it follows the interceptor
I meant inputs - 2

[ngeoffray, 2012-11-21 09:43:41]

Thanks Karl and Johnni. PTAL, I had to do some change to not regress on
performance.

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

https://codereview.chromium.org/11412105/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1324: // We know it's a
selector call if it follows the interceptor
On 2012/11/21 08:34:55, karlklose wrote:
> I meant inputs - 2

Done.

https://codereview.chromium.org/11412105/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1327: return
inputs.length - 1 != selector.argumentCount;
On 2012/11/20 16:40:35, karlklose wrote:
> Trailing whitespace.

Done.

[Johnni Winther, 2012-11-21 11:22:05]

lgtm

[ahe, 2012-11-23 06:51:22]

LGTM

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_array.dart (right):

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_array.dart:172: int get length
=> JS('int', r'#.length', this);
Is the compiler smart enough to elide the return type check in checked mode?

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2511: // interceptor
method and then the actual dynamic call on the
interceptor method -> "getInterceptor" method

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2514: if
(backend.isInterceptorClass(currentElement.getEnclosingClass())
Shouldn't this be part of pushInvokeInterceptor?

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2807:
pushInvokeInterceptor(interceptor, receiver);
Why don't you need to reuse this here?  Is it because an implicit this send
would end in a different visit method?

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3871: if (element ==
null) {
When can this happen?

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

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1714:
visitFieldGet(HFieldGet node) {
What is a HFieldGet? Is that when we know that we can access a field directly?

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1718: push(new
js.PropertyAccess.field(pop(), 'length'), node);
Please add a comment explaining why this name shouldn't be mangled.

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

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1321: accept(HVisitor
visitor);
Remove this method?  I believe it was only there to make this class implicitly
abstract (it is already defined in HInstruction).

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1352: accept(HVisitor
visitor);
Remove this method?

[ngeoffray, 2012-11-23 12:01:16]

Thanks Peter, comments addressed in: https://codereview.chromium.org/11411147/.

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_array.dart (right):

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_array.dart:172: int get length
=> JS('int', r'#.length', this);
On 2012/11/23 06:51:23, ahe wrote:
> Is the compiler smart enough to elide the return type check in checked mode?

Yes. I have just double checked.

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2511: // interceptor
method and then the actual dynamic call on the
On 2012/11/23 06:51:23, ahe wrote:
> interceptor method -> "getInterceptor" method

Done.

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2514: if
(backend.isInterceptorClass(currentElement.getEnclosingClass())
On 2012/11/23 06:51:23, ahe wrote:
> Shouldn't this be part of pushInvokeInterceptor?

Done.

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2807:
pushInvokeInterceptor(interceptor, receiver);
On 2012/11/23 06:51:23, ahe wrote:
> Why don't you need to reuse this here?  Is it because an implicit this send
> would end in a different visit method?

Not sure I understand. The handling of an implicit this send is done line 2802.

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3871: if (element ==
null) {
On 2012/11/23 06:51:23, ahe wrote:
> When can this happen?

In unit tests.

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

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1714:
visitFieldGet(HFieldGet node) {
On 2012/11/23 06:51:23, ahe wrote:
> What is a HFieldGet? Is that when we know that we can access a field directly?

Yes.

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1718: push(new
js.PropertyAccess.field(pop(), 'length'), node);
On 2012/11/23 06:51:23, ahe wrote:
> Please add a comment explaining why this name shouldn't be mangled.

Done.

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

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1321: accept(HVisitor
visitor);
On 2012/11/23 06:51:23, ahe wrote:
> Remove this method?  I believe it was only there to make this class implicitly
> abstract (it is already defined in HInstruction).

Done.

https://codereview.chromium.org/11412105/diff/4017/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1352: accept(HVisitor
visitor);
On 2012/11/23 06:51:23, ahe wrote:
> Remove this method?

Done.