Re-apply issue 11308169: GVN getInterceptor and use the interceptor constant when the type is known.

Re-apply issue 11308169: GVN getInterceptor and use the interceptor constant when the type is known.
Committed: https://code.google.com/p/dart/source/detail?r=15380

[ngeoffray, 2012-11-23 10:27:56]

PTAL. It turns out the optimization revealed two bugs in our code base:

1) The optimistic parameter types: since we were not registering parameter types
for interceptor calls, we were only looking at the types of non-interceptor
calls. So we would optimize every 'add' method with a null parameter if we found
something like: "new StringBuffer().add(null)", even though there were other
(intercepted) calls in the dart code.

2) The type intersection and union: we were saying that "Object(canBeNull)
intersection int(canBeNull)" returns Null. Not good.

[ngeoffray, 2012-11-23 10:28:47]

Also, FYI: The patch set 1 contains the original change. Patch set 2 contains
the fixes for the described problems. Note that it's the optimizations that I
have put in that trigger the bugs.

On 2012/11/23 10:27:56, ngeoffray wrote:
> PTAL. It turns out the optimization revealed two bugs in our code base:
> 
> 1) The optimistic parameter types: since we were not registering parameter
types
> for interceptor calls, we were only looking at the types of non-interceptor
> calls. So we would optimize every 'add' method with a null parameter if we
found
> something like: "new StringBuffer().add(null)", even though there were other
> (intercepted) calls in the dart code.
> 
> 2) The type intersection and union: we were saying that "Object(canBeNull)
> intersection int(canBeNull)" returns Null. Not good.

[ahe, 2012-11-23 11:32:54]

I'm really confused about the whole intersection of types.

https://codereview.chromium.org/11421056/diff/4002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/types.dart (right):

https://codereview.chromium.org/11421056/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:91: bool
isBoundedObject() => false;
This name is really confusing.  I think you said it meant top as known from type
theory.

[ngeoffray, 2012-11-23 11:49:50]

https://codereview.chromium.org/11421056/diff/4002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/types.dart (right):

https://codereview.chromium.org/11421056/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:91: bool
isBoundedObject() => false;
On 2012/11/23 11:32:54, ahe wrote:
> This name is really confusing.  I think you said it meant top as known from
type
> theory.

As discussed, renamed the method to isTop().

[ngeoffray, 2012-11-27 09:41:10]

Friendly ping :-)

[ahe, 2012-11-27 10:19:11]

LGTM

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:463:
staticTypeMap[element] = newTypes;
Seems like the above lines should be a helper method (which returns true if the
types changed).

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:505:
selectorTypeMap[selector] = newTypes;
This method could be reused here.

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2412: bool
typeEquals(other) => other is HInterceptor;
I think it is confusing to use the word "type" in this context.  One could
easily jump to the conclusion that this would be related to HType.

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:269: HInstruction
fromNativeToDynamicInvocation(HInvokeStatic node,
What does native mean in this context?

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/ssa/types.dart (right):

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:785: class
HBoundedPotentialPrimitiveType extends HBoundedType {
How about providing a toString method?

[ahe, 2012-11-27 10:19:57]

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:505:
selectorTypeMap[selector] = newTypes;
On 2012/11/27 10:19:11, ahe wrote:
> This method could be reused here.

By "this method", I probably mean "that method", as in that method I talked
about above.

[ngeoffray, 2012-11-27 10:41:04]

Thanks Peter!

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:463:
staticTypeMap[element] = newTypes;
On 2012/11/27 10:19:11, ahe wrote:
> Seems like the above lines should be a helper method (which returns true if
the
> types changed).

Good refactoring suggestion. Done.

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:505:
selectorTypeMap[selector] = newTypes;
On 2012/11/27 10:19:11, ahe wrote:
> This method could be reused here.

Done.

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:269: HInstruction
fromNativeToDynamicInvocation(HInvokeStatic node,
On 2012/11/27 10:19:11, ahe wrote:
> What does native mean in this context?

It's for a primitive instruction (HIndex, Hadd, ...). I renamed the method to
fromPrimitiveInstructionToDynamicInvocation.

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/ssa/types.dart (right):

https://chromiumcodereview.appspot.com/11421056/diff/13001/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:785: class
HBoundedPotentialPrimitiveType extends HBoundedType {
On 2012/11/27 10:19:11, ahe wrote:
> How about providing a toString method?

Done.