In generated code for x64 don't load object's class directly from class_ field.

In generated code for x64 don't load object's class directly from class_ field.

Instead look it up in class table by class index taken from object's tags.

R=iposva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=8159

[Vyacheslav Egorov (Google), 2012-05-30 16:11:54]

Please take a look.

[srdjan, 2012-05-30 16:44:00]

DBC

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.cc#ne...
runtime/vm/assembler_x64.cc:1564: Register scratch) {
You do not need to pass scratch, use TMP register instead.

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.cc#ne...
runtime/vm/assembler_x64.cc:1572: Register scratch) {
ditto

http://codereview.chromium.org/10458031/diff/1/runtime/vm/flow_graph_compiler...
File runtime/vm/flow_graph_compiler_x64.cc (right):

http://codereview.chromium.org/10458031/diff/1/runtime/vm/flow_graph_compiler...
runtime/vm/flow_graph_compiler_x64.cc:142:
Library::CoreImplLibrary()).LookupClass(class_name));
This should be ZoneHandle as it is escaping its creation scope

http://codereview.chromium.org/10458031/diff/1/runtime/vm/flow_graph_compiler...
runtime/vm/flow_graph_compiler_x64.cc:302: __ LoadClassByIndex(R13, R10);
You could reuse R10 instead of R13, since you are exiting anyway.

[Vyacheslav Egorov (Google), 2012-05-30 17:11:06]

Thanks Srdjan!

Comments addressed, CL updated.

[Ivan Posva, 2012-05-30 17:13:42]

Mostly comments about naming, more to come.

-Ivan

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.h#new...
runtime/vm/assembler_x64.h:524: void LoadClassIndexOfObject(Register result,
Register object);
How about?
LoadClassId(Register result, Register object)

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.h#new...
runtime/vm/assembler_x64.h:526: void LoadClassByIndex(Register result, Register
class_index);
LoadClassById

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.h#new...
runtime/vm/assembler_x64.h:528: void LoadClassOfObject(Register result, Register
object, Register scratch);
LoadClass? Might be a bit too generic, better suggestions appreciated.

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.h#new...
runtime/vm/assembler_x64.h:530: void CompareClassOfObject(Register object,
How about renaming this to:
CompareClassId(Register object, const Class& clazz, Register scratch)

When I read this at use sites I always feel that we are really comparing the
class objects which is not the case.

http://codereview.chromium.org/10458031/diff/1/runtime/vm/flow_graph_compiler...
File runtime/vm/flow_graph_compiler_x64.cc (right):

http://codereview.chromium.org/10458031/diff/1/runtime/vm/flow_graph_compiler...
runtime/vm/flow_graph_compiler_x64.cc:142:
Library::CoreImplLibrary()).LookupClass(class_name));
On 2012/05/30 16:44:00, srdjan wrote:
> This should be ZoneHandle as it is escaping its creation scope

The real issue here is that this function is breaking the agreement and is
returning a handle instead of RawClass*. Because then at the use we can allocate
the correct type of handle.

[Vyacheslav Egorov (Google), 2012-05-30 17:29:57]

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.h#new...
runtime/vm/assembler_x64.h:524: void LoadClassIndexOfObject(Register result,
Register object);
On 2012/05/30 17:13:42, Ivan Posva wrote:
> How about?
> LoadClassId(Register result, Register object)

I am using name "class index" for consistency. It is called ClassIndex
everywhere else (e.g. there is a getter in RawObject that is named
GetClassIndex).

Should I rename class index to class id everywhere?

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.h#new...
runtime/vm/assembler_x64.h:530: void CompareClassOfObject(Register object,
On 2012/05/30 17:13:42, Ivan Posva wrote:
> How about renaming this to:
> CompareClassId(Register object, const Class& clazz, Register scratch)
> 
> When I read this at use sites I always feel that we are really comparing the
> class objects which is not the case.

But it does not accept index. It is implementation detail of this function that
it compares indexes/ids not objects. It should not matter for external user.

I can rename it, but I feel wrong exposing implementation detail through it's
name instead of describing what it does --- compares classes.

[Vyacheslav Egorov (Google), 2012-05-30 17:52:36]

http://codereview.chromium.org/10458031/diff/1/runtime/vm/flow_graph_compiler...
File runtime/vm/flow_graph_compiler_x64.cc (right):

http://codereview.chromium.org/10458031/diff/1/runtime/vm/flow_graph_compiler...
runtime/vm/flow_graph_compiler_x64.cc:142:
Library::CoreImplLibrary()).LookupClass(class_name));
On 2012/05/30 17:13:42, Ivan Posva wrote:
> On 2012/05/30 16:44:00, srdjan wrote:
> > This should be ZoneHandle as it is escaping its creation scope
> 
> The real issue here is that this function is breaking the agreement and is
> returning a handle instead of RawClass*. Because then at the use we can
allocate
> the correct type of handle.

Fixed.

[Ivan Posva, 2012-05-30 18:00:30]

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.h#new...
runtime/vm/assembler_x64.h:524: void LoadClassIndexOfObject(Register result,
Register object);
On 2012/05/30 17:29:57, Vyacheslav Egorov (Google) wrote:
> On 2012/05/30 17:13:42, Ivan Posva wrote:
> > How about?
> > LoadClassId(Register result, Register object)
> 
> I am using name "class index" for consistency. It is called ClassIndex
> everywhere else (e.g. there is a getter in RawObject that is named
> GetClassIndex).
> 
> Should I rename class index to class id everywhere?

I can change the use of index to id in other areas of the VM.

http://codereview.chromium.org/10458031/diff/1/runtime/vm/assembler_x64.h#new...
runtime/vm/assembler_x64.h:530: void CompareClassOfObject(Register object,
On 2012/05/30 17:29:57, Vyacheslav Egorov (Google) wrote:
> On 2012/05/30 17:13:42, Ivan Posva wrote:
> > How about renaming this to:
> > CompareClassId(Register object, const Class& clazz, Register scratch)
> > 
> > When I read this at use sites I always feel that we are really comparing the
> > class objects which is not the case.
> 
> But it does not accept index. It is implementation detail of this function
that
> it compares indexes/ids not objects. It should not matter for external user.
> 
> I can rename it, but I feel wrong exposing implementation detail through it's
> name instead of describing what it does --- compares classes.

Understand the consistency argument about passing a class. Instead it would make
sense to pass an id.

[Ivan Posva, 2012-05-30 18:15:19]

Probably last set of comments...

-Ivan

http://codereview.chromium.org/10458031/diff/7008/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_x64.cc (right):

http://codereview.chromium.org/10458031/diff/7008/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:238: // R10: instance class to check.
// R10: instance class id to check.

http://codereview.chromium.org/10458031/diff/7008/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:239: void
FlowGraphCompiler::CheckClasses(const GrowableArray<const Class*>& classes,
How about CheckClassIds and passing a GrowableArray<int> instead? That way you
are certain that the handles are not surviving past the assembly stage.

http://codereview.chromium.org/10458031/diff/7008/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:313: const Class& mint_class =
Class::Handle(object_store->mint_class());
kMint

http://codereview.chromium.org/10458031/diff/7008/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:314: const Class& bigint_class =
Class::Handle(object_store->bigint_class());
kBigInt

http://codereview.chromium.org/10458031/diff/7008/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:315: const Class& double_class =
Class::Handle(object_store->double_class());
kDouble

http://codereview.chromium.org/10458031/diff/7008/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:331: const Class& one_byte_string_class =
ditto, here and below...

http://codereview.chromium.org/10458031/diff/7008/runtime/vm/stub_code_ia32.cc
File runtime/vm/stub_code_ia32.cc (left):

http://codereview.chromium.org/10458031/diff/7008/runtime/vm/stub_code_ia32.c...
runtime/vm/stub_code_ia32.cc:1594: // EAX: receiver's class.
Thanks!

[Vyacheslav Egorov (Google), 2012-05-30 18:30:47]

Renamed assembler methods on both ia32 and x64. 

Made methods more id centric (CompareClassId vs. CompareClass, CheckClassIds vs.
CheckClasses).

Please take another look.

[Vyacheslav Egorov (Google), 2012-05-30 19:01:09]

Killed CoreClass both on ia32/x64 and switched to class ids for array classes.

[Vyacheslav Egorov (Google), 2012-05-31 08:49:02]

ping

[Ivan Posva, 2012-05-31 12:50:55]

LGTM -ip