Add deopt info to code object and print it

Add deopt info to code object and print it. Still missing is the code to 'execute' the deopt instructions and thus deoptimize.
Committed: https://code.google.com/p/dart/source/detail?r=10308

[siva, 2012-08-04 01:21:50]

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/compile...
File runtime/vm/compiler.cc (right):

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/compile...
runtime/vm/compiler.cc:329: OS::Print("\n}\n");
Could you move the \n to %s\n" above so that this is uniform like other places
as OS::Print("}\n");

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
File runtime/vm/deopt_instructions.cc (right):

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.cc:14: // 'from_index'. 'from_index' represents
the local count >= 0, first
'from_index' to specified slots in the unoptimized frame.
'from_index' represents the ......

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.cc:196: object_table_->Add(&obj);
Need to assert obj is a Zone handle because you are storing away a pointer to it
in the table.

This is one of those cases where I think we should pass in a pointer to a handle
as the parameter because we are going to store it away.

Another option is to create a GrowableObjectArray object and  store the
RawObject directly in it. This GrowableObjectArray would be assigned to the code
object using MakeArray

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.cc:218: void DeoptInfoBuilder::AddCopy(const
Location& loc, const Value& value) {
The "to index" is implicit and you have made a comment that these instructions
should be added in a specific order so that the "to indexes" come out correctly.
I am wondering if it makes sense to add a "to_index" parameter to this function
so that you can assert that the intended "to_index" matches the actual slot it
ends up in the array.

It is possible to make silly order mistakes and then debugging that could be a
nightmare.

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
File runtime/vm/deopt_instructions.h (right):

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.h:29: kCallerPc,
These names don't seem to convey the intent that the instruction is setting
something, how about
kSetRetAddress
kSetStackslot
kSetPCMarker
etc.

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.h:35: virtual intptr_t from_index() const = 0;
It seems too heavy weight to have these methods as virtual functions, why not
put the fields
intptr_t from_index_;
Kind kind;
here in the base class and call the base constructor
: DeoptInstr(kind, from_index) in the sub class constructors.

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.h:42: virtual const char* ToCString() const = 0;
The public part should come first in the class right (per C++ coding style).

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.h:55: // 'object_table' hold all objects referred
to by DeoptInstr in
holds

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.h:56: // all of DeoptInfo instances for a single
Code object.
all the DeoptInfo instances ....

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.h:71: RawDeoptInfo* CreateDeoptInfo();
RawDeoptInfo* CreateDeoptInfo() const;

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/flow_gr...
File runtime/vm/flow_graph_compiler.cc (right):

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/flow_gr...
runtime/vm/flow_graph_compiler.cc:281: }
If you had been carrying the object_table_ as a growable object array then this
copying would not be needed and all the handle creation that is going on for it
could also have been avoided.

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/locatio...
File runtime/vm/locations.h (right):

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/locatio...
runtime/vm/locations.h:37: 
Similar comment about private: coming before public:

[srdjan, 2012-08-06 20:09:04]

Please take another look.
(Also includes a small fix in intermediate_language.h)

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/compile...
File runtime/vm/compiler.cc (right):

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/compile...
runtime/vm/compiler.cc:329: OS::Print("\n}\n");
On 2012/08/04 01:21:50, asiva wrote:
> Could you move the \n to %s\n" above so that this is uniform like other places
> as OS::Print("}\n");

Done.

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
File runtime/vm/deopt_instructions.cc (right):

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.cc:14: // 'from_index'. 'from_index' represents
the local count >= 0, first
On 2012/08/04 01:21:50, asiva wrote:
> 'from_index' to specified slots in the unoptimized frame.
> 'from_index' represents the ......

Done.

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.cc:196: object_table_->Add(&obj);
On 2012/08/04 01:21:50, asiva wrote:
> Need to assert obj is a Zone handle because you are storing away a pointer to
it
> in the table.
> 
> This is one of those cases where I think we should pass in a pointer to a
handle
> as the parameter because we are going to store it away.
> 
> Another option is to create a GrowableObjectArray object and  store the
> RawObject directly in it. This GrowableObjectArray would be assigned to the
code
> object using MakeArray

Discussed and switched to GrowableObjectAray.

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.cc:218: void DeoptInfoBuilder::AddCopy(const
Location& loc, const Value& value) {
On 2012/08/04 01:21:50, asiva wrote:
> The "to index" is implicit and you have made a comment that these instructions
> should be added in a specific order so that the "to indexes" come out
correctly.
> I am wondering if it makes sense to add a "to_index" parameter to this
function
> so that you can assert that the intended "to_index" matches the actual slot it
> ends up in the array.
> 
> It is possible to make silly order mistakes and then debugging that could be a
> nightmare. 

Done.

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
File runtime/vm/deopt_instructions.h (right):

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.h:29: kCallerPc,
On 2012/08/04 01:21:50, asiva wrote:
> These names don't seem to convey the intent that the instruction is setting
> something, how about
> kSetRetAddress
> kSetStackslot
> kSetPCMarker
> etc.

Did a mixture of kSet and kCopy, e.g., kCopyRegister

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.h:35: virtual intptr_t from_index() const = 0;
On 2012/08/04 01:21:50, asiva wrote:
> It seems too heavy weight to have these methods as virtual functions, why not
> put the fields
> intptr_t from_index_;
> Kind kind;
> here in the base class and call the base constructor
> : DeoptInstr(kind, from_index) in the sub class constructors.

Discussed and left it like that. Let's see once the copying code (instruction
'execution') is in if the virtual are justified (as I think they are).

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.h:42: virtual const char* ToCString() const = 0;
On 2012/08/04 01:21:50, asiva wrote:
> The public part should come first in the class right (per C++ coding style).

Done.

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.h:55: // 'object_table' hold all objects referred
to by DeoptInstr in
On 2012/08/04 01:21:50, asiva wrote:
> holds

Done.

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/deopt_i...
runtime/vm/deopt_instructions.h:56: // all of DeoptInfo instances for a single
Code object.
On 2012/08/04 01:21:50, asiva wrote:
> all the DeoptInfo instances ....

Done.

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/flow_gr...
File runtime/vm/flow_graph_compiler.cc (right):

https://chromiumcodereview.appspot.com/10823131/diff/11004/runtime/vm/flow_gr...
runtime/vm/flow_graph_compiler.cc:281: }
On 2012/08/04 01:21:50, asiva wrote:
> If you had been carrying the object_table_ as a growable object array then
this
> copying would not be needed and all the handle creation that is going on for
it
> could also have been avoided. 

Done.

[siva, 2012-08-06 20:32:24]

LGTM.

Thanks.

https://chromiumcodereview.appspot.com/10823131/diff/5006/runtime/vm/flow_gra...
File runtime/vm/flow_graph_compiler.cc (right):

https://chromiumcodereview.appspot.com/10823131/diff/5006/runtime/vm/flow_gra...
runtime/vm/flow_graph_compiler.cc:274:
DeoptInfo::Handle(deopt_stubs_[i]->CreateDeoptInfo(this));
The handle could be created outside the loop?

[srdjan, 2012-08-06 20:39:47]

https://chromiumcodereview.appspot.com/10823131/diff/5006/runtime/vm/flow_gra...
File runtime/vm/flow_graph_compiler.cc (right):

https://chromiumcodereview.appspot.com/10823131/diff/5006/runtime/vm/flow_gra...
runtime/vm/flow_graph_compiler.cc:274:
DeoptInfo::Handle(deopt_stubs_[i]->CreateDeoptInfo(this));
On 2012/08/06 20:32:24, asiva wrote:
> The handle could be created outside the loop?

Done.

[srdjan, 2012-08-06 20:39:48]

https://chromiumcodereview.appspot.com/10823131/diff/5006/runtime/vm/flow_gra...
File runtime/vm/flow_graph_compiler.cc (right):

https://chromiumcodereview.appspot.com/10823131/diff/5006/runtime/vm/flow_gra...
runtime/vm/flow_graph_compiler.cc:274:
DeoptInfo::Handle(deopt_stubs_[i]->CreateDeoptInfo(this));
On 2012/08/06 20:32:24, asiva wrote:
> The handle could be created outside the loop?

Done.