Issue 10209002: Progress toward inlined type checks for classes with type arguments: factor out code, optimize typ…

Issue 10209002: Progress toward inlined type checks for classes with type arguments: factor out code, optimize typ… (Closed)

Created:
8 years, 8 months ago by srdjan

Modified:
8 years, 8 months ago

Reviewers:
regis

CC:
reviews_dartlang.org, vm-dev_dartlang.org

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/

Visibility:
Public.

More Reviews

Description

Progress toward inlined type checks for classes with type arguments: factor out code, optimize type arguments of an instance. Committed: https://code.google.com/p/dart/source/detail?r=6946

Patch Set 8 : #

Total comments: 17

Created: 8 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+256 lines, -161 lines)			Patch
M	runtime/vm/code_generator.cc	View	1 2 3 4 5 6 7	5 chunks	+148 lines, -116 lines	8 comments	Download
M	runtime/vm/code_generator_ia32.h	View	1 2 3 4 5	2 chunks	+8 lines, -1 line	0 comments	Download
M	runtime/vm/code_generator_ia32.cc	View	1 2 3 4 5	6 chunks	+67 lines, -29 lines	0 comments	Download
M	runtime/vm/object.h	View	1 2 3 4 5	2 chunks	+4 lines, -0 lines	2 comments	Download
M	runtime/vm/object.cc	View	1 2 3 4 5	1 chunk	+26 lines, -0 lines	4 comments	Download
M	runtime/vm/stub_code_ia32.cc	View	1 2 3 4 5	4 chunks	+3 lines, -15 lines	3 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

regis

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/code_generator.cc File runtime/vm/code_generator.cc (right): https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/code_generator.cc#newcode344 runtime/vm/code_generator.cc:344: const Type& instance_type = Type::Handle(instance.GetType()); GetType allocates a new ...

8 years, 8 months ago (2012-04-25 01:53:53 UTC) #2

srdjan

8 years, 8 months ago (2012-04-25 16:38:01 UTC) #3

Unfortunately lost the CL, will resume from the a new, restored one which
contains all your suggestions below, and will attempt to skip canonicalization.

Yes, I plan to do it for x64 once I figured it out on ia32.

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/code_ge...
File runtime/vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/code_ge...
runtime/vm/code_generator.cc:344: const Type& instance_type =
Type::Handle(instance.GetType());
On 2012/04/25 01:53:53, regis wrote:
> GetType allocates a new type, but you do not really need it. You can extract
the
> type class and the type arguments separately.

Done.

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/code_ge...
runtime/vm/code_generator.cc:353: for (int i = 0; i <  type_arguments.Length();
i++) {
On 2012/04/25 01:53:53, regis wrote:
> 2 spaces

Done.

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/code_ge...
runtime/vm/code_generator.cc:358: UNIMPLEMENTED();
On 2012/04/25 01:53:53, regis wrote:
> I think you only miss IsTypeParameter, which could occur when checking
function
> types that are not instantiated yet. You could then replace UNIMPLEMENTED by
> UNREACHABLE.
> However, I think that tests involving uninstantiated function types would only
> occur at compile time, not a run time, so they would not show up in this
> optimizing routine (I think, but I may be wrong).

UNREACHABLE and added comment:
// type_at cannot be TypeParameter at runtime.

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/code_ge...
runtime/vm/code_generator.cc:373: // Note that the 'result' contains the whole
type test (inlcluding type
On 2012/04/25 01:53:53, regis wrote:
> including

Done.

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/object....
runtime/vm/object.cc:2751: if (IsFinalized() !=
other_instantiated.IsFinalized()) {
On 2012/04/25 01:53:53, regis wrote:
> I do not think this is necessary. An InstantiatedType is only created at run
> time and finalization has meaning at compile time only. We mark an
> InstantiatedType as finalized upon creation. 

Is is used for canonicalization. Will try to do it without canonicalization.

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/object....
runtime/vm/object.cc:2760: if (instantiator_type_arguments() !=
On 2012/04/25 01:53:53, regis wrote:
> Using != is not correct. You should be using Equals.

Using AreEqual.

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/object.h
File runtime/vm/object.h (right):

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/object....
runtime/vm/object.h:1048: virtual bool Equals(const AbstractType& other) const;
On 2012/04/25 01:53:53, regis wrote:
> Why do you need this?

Canonicalization ... will try without.

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/stub_co...
File runtime/vm/stub_code_ia32.cc (right):

https://chromiumcodereview.appspot.com/10209002/diff/11007/runtime/vm/stub_co...
runtime/vm/stub_code_ia32.cc:1761: // account.
On 2012/04/25 01:53:53, regis wrote:
> Should you mention that instances of type Smi cannot be tested by this stub? 

Done.

Expand Messages | Collapse Messages