Cleanup after the interceptor refactoring work, and add a new SSA instruction for an interceptor, s…

Cleanup after the interceptor refactoring work, and add a new SSA instruction for an interceptor, soon to be optimized.
Committed: https://code.google.com/p/dart/source/detail?r=15202

[Johnni Winther, 2012-11-21 13:36:56]

lgtm

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

https://codereview.chromium.org/11348177/diff/12/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2409:
Set<ClassElement> interceptedClasses = getInterceptedClassesOn(send, selector);
Long line

[ngeoffray, 2012-11-21 13:41:04]

Thanks Johnni

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

https://codereview.chromium.org/11348177/diff/12/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2409:
Set<ClassElement> interceptedClasses = getInterceptedClassesOn(send, selector);
On 2012/11/21 13:36:56, Johnni Winther wrote:
> Long line

Done.

[karlklose, 2012-11-21 14:45:52]

LGTM!

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

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2413: if
(interceptedClasses != null) {
"|| interceptedClasses.isEmpty" to be sure?

[karlklose, 2012-11-21 14:45:53]

LGTM!

[ahe, 2012-11-22 13:26:11]

I'm not sure I understand all the changes you have made.

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

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:752:
Set<ClassElement> getInterceptedClassesOn(Selector selector) {
How about computing the reverse mapping?  This seems like a pretty expensive
operation.

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

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2413: if
(interceptedClasses != null) {
On 2012/11/21 14:45:53, karlklose wrote:
> "|| interceptedClasses.isEmpty" to be sure?

An assertion would be more appropriate.  The documentation clearly states that
it returns null if it is empty.

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (left):

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1420: void
prepareGvn(HTypeMap types) {
What is this change for?

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

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2402: String toString()
=> 'interceptor on $interceptedClasses';
What is the method name?

[ngeoffray, 2012-11-22 13:37:59]

Thanks for taking a look Peter.

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

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:752:
Set<ClassElement> getInterceptedClassesOn(Selector selector) {
On 2012/11/22 13:26:11, ahe wrote:
> How about computing the reverse mapping?  This seems like a pretty expensive
> operation.

As discussed, we already have the reverse mapping, which is interceptedElements.
The loop is about finding elements that match the selector.

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (left):

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1420: void
prepareGvn(HTypeMap types) {
On 2012/11/22 13:26:11, ahe wrote:
> What is this change for?

This code was temporary, to make sure we did not regress on some benchmarks
(otherwise the getInterceptor method would be treated as having side effects and
therefore disabling potential gvn optimizations).

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

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2402: String toString()
=> 'interceptor on $interceptedClasses';
On 2012/11/22 13:26:11, ahe wrote:
> What is the method name?

An interceptor is not necessary on one method call. It can be shared between
different intercepted calls.

[ahe, 2012-11-22 13:45:38]

LGTM!  It is really nice to see that you pushed this all the way!

[karlklose, 2012-11-22 15:30:53]

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/tracer.dart (right):

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/tracer.dart:319: return
"Intercept: $receiver";
'$receiver' -> '$value'.

[ngeoffray, 2012-11-22 15:42:00]

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/tracer.dart (right):

https://codereview.chromium.org/11348177/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/tracer.dart:319: return
"Intercept: $receiver";
On 2012/11/22 15:30:53, karlklose wrote:
> '$receiver' -> '$value'.

Nice catch. https://codereview.chromium.org/11414131