Inline type check where type argument that we check against is instantiated.

Inline type check where type argument that we check against is instantiated.
Committed: https://code.google.com/p/dart/source/detail?r=7046

[regis, 2012-04-26 00:36:17]

Either this new inlined code is not tested or I miss something.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
File runtime/vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:413: if (!is_raw_type && !type_arguments.IsNull())
{
No need to check && !type_arguments.IsNull()

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:415: // or if it's class has not been resolved
(malformed type).
its class

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:415: // or if it's class has not been resolved
(malformed type).
I do not think you can encounter a malformed type here, because the runtime call
would have thrown an exception.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:417: // We can handle only one argument so far
far.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:420: const AbstractType& type_argument_0 =
type_argument instead of type_argument_0?

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:421:
AbstractType::ZoneHandle(type_arguments.TypeAt(0));
Why a ZoneHandle?

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:422: if (type_argument_0.IsType() &&
!type_argument_0.HasResolvedTypeClass()) {
Are you checking for IsMalformed()?
But how can this happen? I thought you do not update the cache if the runtime
call throws a TypeError.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
File runtime/vm/code_generator_ia32.cc (right):

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1531: // Inline checks for one type-argument
only.
one type argument

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1535: const AbstractType& type_at_0 =
type_argument instead of type_at_0?

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1537: if (!type_at_0.IsType() ||
!type_at_0.HasResolvedTypeClass()) {
We already checked for type.IsMalformed() in the caller. What case are you
handling here?

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1546: GenerateClassTestCache(node_id,
token_index, type_class,
I do not understand this shortcut.
Wouldn't this inlined code wrongly link to is_not_instance_lbl when testing an
instance of type A[Object] against B[Object]?
This would be wrong with this declaration:
class A extends B<Object>
Oh, wait, your GenerateClassTestCache is not a class equality test but a
subclass test. How about renaming it to GenerateSubclassTestCache?

However, this is not correct with interfaces, I think.
Consider class A<T> implements I<Object>

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1553: GenerateClassTestCache(node_id,
token_index, type_class,
The same argument applies here. Consider
class A<T> implements I<bool>

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1565: __ j(EQUAL, is_instance_lbl);
I do not understand this either. Consider
class A implements I<bool>
new A() is I<String> will say true, which is wrong.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1575: __ j(EQUAL, is_instance_lbl);
Consider
class A<T> implements I<bool>
new A() is I<String> will say true, which is wrong.

[srdjan, 2012-04-26 18:08:37]

Still work in progress

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
File runtime/vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:413: if (!is_raw_type && !type_arguments.IsNull())
{
On 2012/04/26 00:36:17, regis wrote:
> No need to check && !type_arguments.IsNull()

Done.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:415: // or if it's class has not been resolved
(malformed type).
On 2012/04/26 00:36:17, regis wrote:
> I do not think you can encounter a malformed type here, because the runtime
call
> would have thrown an exception.

I misunderstood your explanation of malformed type. Changed to assert and added
comment why we cannot encounter malformed class

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:417: // We can handle only one argument so far
On 2012/04/26 00:36:17, regis wrote:
> far.

Done.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:420: const AbstractType& type_argument_0 =
On 2012/04/26 00:36:17, regis wrote:
> type_argument instead of type_argument_0?

Removed that code.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:421:
AbstractType::ZoneHandle(type_arguments.TypeAt(0));
On 2012/04/26 00:36:17, regis wrote:
> Why a ZoneHandle?

Handle

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator.cc:422: if (type_argument_0.IsType() &&
!type_argument_0.HasResolvedTypeClass()) {
On 2012/04/26 00:36:17, regis wrote:
> Are you checking for IsMalformed()?
> But how can this happen? I thought you do not update the cache if the runtime
> call throws a TypeError.

Converted to assert added comment why it can't be malformed.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
File runtime/vm/code_generator_ia32.cc (right):

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1531: // Inline checks for one type-argument
only.
On 2012/04/26 00:36:17, regis wrote:
> one type argument

Done.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1535: const AbstractType& type_at_0 =
On 2012/04/26 00:36:17, regis wrote:
> type_argument instead of type_at_0?

As discussed: type_argument sounds too close to type_arguments, renaming to
tp_argument.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1537: if (!type_at_0.IsType() ||
!type_at_0.HasResolvedTypeClass()) {
On 2012/04/26 00:36:17, regis wrote:
> We already checked for type.IsMalformed() in the caller. What case are you
> handling here?

Changing to assert.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1546: GenerateClassTestCache(node_id,
token_index, type_class,
On 2012/04/26 00:36:17, regis wrote:
> I do not understand this shortcut.
> Wouldn't this inlined code wrongly link to is_not_instance_lbl when testing an
> instance of type A[Object] against B[Object]?
> This would be wrong with this declaration:
> class A extends B<Object>
> Oh, wait, your GenerateClassTestCache is not a class equality test but a
> subclass test. How about renaming it to GenerateSubclassTestCache?
> 
> However, this is not correct with interfaces, I think.
> Consider class A<T> implements I<Object>

Made simpler comment to GenerateClassTestCache. That should explain the question
about interface. Rename as you suggested (offline):
GenerateSubtypeTestCacheLookup

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1553: GenerateClassTestCache(node_id,
token_index, type_class,
On 2012/04/26 00:36:17, regis wrote:
> The same argument applies here. Consider
> class A<T> implements I<bool>

Discussed and answered offline, I guess.

https://chromiumcodereview.appspot.com/10134069/diff/4001/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1565: __ j(EQUAL, is_instance_lbl);
On 2012/04/26 00:36:17, regis wrote:
> I do not understand this either. Consider
> class A implements I<bool>
> new A() is I<String> will say true, which is wrong.

You are right. Nifty example, and there is no test for that, yet.  Adding test
in a different CL. Offline discussion concluded that we cannot do inlined tests
at all. Need to chat about it more.

[srdjan, 2012-04-26 21:26:39]

Regis, can you please have another look. The main change is to limit the inlined
checks only to instances of GrowableObjectArray and ObjectArray until we have
figured out a better solution. This change gives still 3x boost to DeltaBlue,
but not much to dart2js.

[regis, 2012-04-26 21:55:05]

LGTM

https://chromiumcodereview.appspot.com/10134069/diff/9002/runtime/vm/code_gen...
File runtime/vm/code_generator_ia32.cc (right):

https://chromiumcodereview.appspot.com/10134069/diff/9002/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1531: // Note that the test below must be
synced withe the tests in
with

https://chromiumcodereview.appspot.com/10134069/diff/9002/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1532: // CodeGenerator::UpdateTestCache.
You could factorize the tests in an helper. This way, it would stay in sync.

https://chromiumcodereview.appspot.com/10134069/diff/9002/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1561: // otehrwise.
otherwise

https://chromiumcodereview.appspot.com/10134069/diff/9002/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1565: // type-parameters are simple structure.
I do not quite understand this TODO, but it has your name on it :-)

[srdjan, 2012-04-26 22:05:48]

https://chromiumcodereview.appspot.com/10134069/diff/9002/runtime/vm/code_gen...
File runtime/vm/code_generator_ia32.cc (right):

https://chromiumcodereview.appspot.com/10134069/diff/9002/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1531: // Note that the test below must be
synced withe the tests in
On 2012/04/26 21:55:05, regis wrote:
> with

Done.

https://chromiumcodereview.appspot.com/10134069/diff/9002/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1532: // CodeGenerator::UpdateTestCache.
On 2012/04/26 21:55:05, regis wrote:
> You could factorize the tests in an helper. This way, it would stay in sync.

Will see to it in a next CL...

https://chromiumcodereview.appspot.com/10134069/diff/9002/runtime/vm/code_gen...
runtime/vm/code_generator_ia32.cc:1565: // type-parameters are simple structure.
On 2012/04/26 21:55:05, regis wrote:
> I do not quite understand this TODO, but it has your name on it :-)

Just an  ideas that I plan to sketch out. Maybe nonsensical.