Collect type feedback for fields.

Collect type feedback for fields.

BUG=

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

[Vyacheslav Egorov (Google), 2013-03-11 17:59:26]

Please start taking a look.

[Kevin Millikin (Google), 2013-03-12 12:14:56]

Overall, I think the big picture is the right way to do it.  I have not looked
closely at the changes to (raw_)object and the generated code.

Here is a first round of superficial comments.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:127: if (field->guarded_cid() == kDynamicCid) continue;
It's slightly nicer to write:

if (load_field == NULL) continue;
Field* field = load_field->field();
if ((field == NULL) || (field->guarded_cid() == kDynamicCid)) continue;

https://codereview.chromium.org/12529008/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:137: if (!found) {
if (!found) result->Add(field);

to match the style you used earlier.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:230: guarded_fields =
CollectGuardedFieldLoads(flow_graph);
CollectGuardedFieldLoads might be better as a member function on FlowGraph.

Does it need to be here in the pipeline?  I would think as late as possible
would be better.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:298: if (FLAG_propagate_types) {
I think this (and also the other site) needs a comment about what propagate
types will optimize and why we chose this position in the pipeline.  Otherwise
we won't be able to figure it out (and why it's duplicated) in a month.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:350: Array::New(guarded_fields->length(), Heap::kOld));
To fit the pattern, this should be a FinalizeGuardedFieldLoads or some such
member function on the compiler class.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_builder.cc
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_builder...
runtime/vm/flow_graph_builder.cc:2598: StoreInstanceFieldInstr* store = new
StoreInstanceFieldInstr(
Thanks, I guess.  But the dangling "(" is the really ugly part to my eyes (a
symptom of breaking at a higher precedence operator when breaking at a lower
precedence one would do).  Try:

StoreInstanceFieldInstr* store =
    new StoreInstanceFieldInstr(node->field(),
                                for_instance.value(),
                                store_value,
                                kEmitStoreBarrier);

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_ia32.cc:634: if (instr->IsStoreInstanceField()) {
else if

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1889: if (instr->field().guarded_cid() !=
kDynamicCid) {
I think it's a bit better to always insert the guards in the builder.  Maybe
it's not easy for some reason?

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
runtime/vm/flow_graph_type_propagator.cc:149: if ((compare == NULL) ||
!compare->InputAt(1)->BindsToConstantNull()) {
I find it easier to read with the (non-virtual) accessors right() and left().

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
runtime/vm/flow_graph_type_propagator.cc:204: if (!current->IsNone() &&
(current->ToCid() == cid)) return;
I find the early return guarding a single statement to be a bit disconcerting.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
runtime/vm/flow_graph_type_propagator.cc:213: (current->is_nullable() ==
CompileType::kNonNullable)) {
Also here.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
File runtime/vm/flow_graph_type_propagator.h (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
runtime/vm/flow_graph_type_propagator.h:31: 
Huh?  I liked it better before.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
runtime/vm/flow_graph_type_propagator.h:53: GrowableArray<bool> visited_;
BitVector?

https://codereview.chromium.org/12529008/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1593: template<typename LeftT, typename
RightT>
Spell out Type like below, or at least be uniform.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1608: const Field& field,
The templates are kind of messy because you have to track data and type flow
through here to figure out what code is actually generated.

I don't have any better solution.

You could at least align the parameters properly here and below.

[Vyacheslav Egorov (Google), 2013-03-12 13:00:27]

Switched field dependencies tracking to use WeakProperty instead of
bidirectional linking.

[Vyacheslav Egorov (Google), 2013-03-12 16:54:40]

I have addressed first round of Kevin's comments.

The biggest change is that GuardField for StoreInstanceField is generated by the
builder itself and not detached by the optimizer.

StoreInstanceField returned to being a simple store as it was.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:127: if (field->guarded_cid() == kDynamicCid) continue;
On 2013/03/12 12:14:56, kmillikin wrote:
> It's slightly nicer to write:
> 
> if (load_field == NULL) continue;
> Field* field = load_field->field();
> if ((field == NULL) || (field->guarded_cid() == kDynamicCid)) continue;

Done.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:137: if (!found) {
On 2013/03/12 12:14:56, kmillikin wrote:
> if (!found) result->Add(field);
> 
> to match the style you used earlier.

Aligned all conditions here to use the same line breaking style.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:230: guarded_fields =
CollectGuardedFieldLoads(flow_graph);
On 2013/03/12 12:14:56, kmillikin wrote:
> CollectGuardedFieldLoads might be better as a member function on FlowGraph.
> 

Moved to FlowGraph.

> Does it need to be here in the pipeline?  I would think as late as possible
> would be better.

Doing it too late might lead to a situtation where load field disappeared from
the graph after it was used for optimization. 

Canonical example would be:

if (this.foo /* not null field foo */ == null) {
  return;
}

load of x.foo should completely disappear (it does not disappear now, but it can
in the future).

https://codereview.chromium.org/12529008/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:350: Array::New(guarded_fields->length(), Heap::kOld));
On 2013/03/12 12:14:56, kmillikin wrote:
> To fit the pattern, this should be a FinalizeGuardedFieldLoads or some such
> member function on the compiler class.

It does not set anything on the code itself now, so I would prefer to leave it
here.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_builder.cc
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_builder...
runtime/vm/flow_graph_builder.cc:2598: StoreInstanceFieldInstr* store = new
StoreInstanceFieldInstr(
On 2013/03/12 12:14:56, kmillikin wrote:
> Thanks, I guess.  But the dangling "(" is the really ugly part to my eyes (a
> symptom of breaking at a higher precedence operator when breaking at a lower
> precedence one would do).  Try:
> 
> StoreInstanceFieldInstr* store =
>     new StoreInstanceFieldInstr(node->field(),
>                                 for_instance.value(),
>                                 store_value,
>                                 kEmitStoreBarrier);

Done.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler_ia32.cc:634: if (instr->IsStoreInstanceField()) {
On 2013/03/12 12:14:56, kmillikin wrote:
> else if

Done.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1889: if (instr->field().guarded_cid() !=
kDynamicCid) {
On 2013/03/12 12:14:56, kmillikin wrote:
> I think it's a bit better to always insert the guards in the builder.  Maybe
> it's not easy for some reason?

Done. Though it requires an expression temp to preserve value and leads to a
slightly worse code in the non-optimized case.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
runtime/vm/flow_graph_type_propagator.cc:149: if ((compare == NULL) ||
!compare->InputAt(1)->BindsToConstantNull()) {
On 2013/03/12 12:14:56, kmillikin wrote:
> I find it easier to read with the (non-virtual) accessors right() and left().

Done.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
runtime/vm/flow_graph_type_propagator.cc:204: if (!current->IsNone() &&
(current->ToCid() == cid)) return;
On 2013/03/12 12:14:56, kmillikin wrote:
> I find the early return guarding a single statement to be a bit disconcerting.

Done.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
runtime/vm/flow_graph_type_propagator.cc:213: (current->is_nullable() ==
CompileType::kNonNullable)) {
On 2013/03/12 12:14:56, kmillikin wrote:
> Also here.

Done.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
File runtime/vm/flow_graph_type_propagator.h (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
runtime/vm/flow_graph_type_propagator.h:31: 
On 2013/03/12 12:14:56, kmillikin wrote:
> Huh?  I liked it better before.

Done.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/flow_graph_type_pr...
runtime/vm/flow_graph_type_propagator.h:53: GrowableArray<bool> visited_;
On 2013/03/12 12:14:56, kmillikin wrote:
> BitVector?

Yeah, that was stupid :-)

https://codereview.chromium.org/12529008/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/12529008/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1593: template<typename LeftT, typename
RightT>
On 2013/03/12 12:14:56, kmillikin wrote:
> Spell out Type like below, or at least be uniform.

Done.

https://codereview.chromium.org/12529008/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:1608: const Field& field,
On 2013/03/12 12:14:56, kmillikin wrote:
> The templates are kind of messy because you have to track data and type flow
> through here to figure out what code is actually generated.
> 
> I don't have any better solution.
> 
> You could at least align the parameters properly here and below.

Done.

[Kevin Millikin (Google), 2013-03-13 16:03:36]

Thanks.  Another round of comments on the generated code.  I still find the
template disturbing --- probably because some of the functions generate no code
or unconditional jumps depending on the template parameters.  It's hard to
reason about the relationships between the types with helpers nested so deeply.

https://codereview.chromium.org/12529008/diff/12001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/12529008/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1337: 
There is an extra blank line.

https://codereview.chromium.org/12529008/diff/12001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/12529008/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1538: template<typename
ValueCidOperandType>
I'm still not liking the templates.  As the writer, it's probably great to avoid
duplication.  But as the reader, I would rather see more explicitly the code
that will be generated.

I don't think it's so bad to break down all the cases.  Correct me if my
reasoning here is wrong.

There are three type parameters, value, field, and nullability.  Value can be
register or an immediate, field and nullability are always the same type---both
immediate or both address---so there are only four possibilities.  One
possibility, immediate value and immediate field, is impossible because it
represents a statically known value type and a statically expected field type. 
The statically known value type can only happen in optimized code, but in that
case we have either definitely eliminated the guard or compiled an unconditional
deopt.

So there are three possibilities:

1. Statically known expected field type, unknown value type, not optimizing
2. Not statically known expected field type, statically known value type,
optimizing
3. Not statically known expected field type, unknown value type, may or may not
be optimizing

If that's right, maybe we could try a pair of functions: one for case 1 (static
cid guard) and one templated over the value operand type for case 2 (dynamic cid
guard).

https://codereview.chromium.org/12529008/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1540: ValueCidOperandType value_cid,
Indentation is screwy.

https://codereview.chromium.org/12529008/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1550: static void
EmitCidGuardInit(FlowGraphCompiler* compiler,
According to the suggestion above, you could inline this code.

https://codereview.chromium.org/12529008/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1553: const FieldAddress& nullability,
Indentation is screwy.

https://codereview.chromium.org/12529008/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1562: __ jmp(ok);
In the case that deopt != NULL, then this will emit a jmp to ok and the caller
will immediately Bind(&ok).  Move the jmp into the if below?

https://codereview.chromium.org/12529008/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1569: static void
CompareJump(FlowGraphCompiler* compiler,
I don't think this one is necessary.

https://codereview.chromium.org/12529008/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1616: CompareJump(compiler, NOT_EQUAL,
nullability, value_cid, deopt);
I think this case is impossible.  The caller has already handled deopt != NULL
and field_cid != kIllegalCid.

https://codereview.chromium.org/12529008/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1708: 
There is an extra blank line here.

https://codereview.chromium.org/12529008/diff/12001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1715: Label* deopt =
compiler->is_optimizing() && CanDeoptimize() ?
CanDeoptimize() returns the constant true, or am I missing something?  If I'm
right, maybe this code can be simplified.

[Vyacheslav Egorov (Google), 2013-03-14 20:36:57]

I have rewritten instruction pattern on ia32 to manually generate all
possibilities. 

(I am leaving porting it to x64 for later, when we figure out what we want
here).

Please take a look.

Control flow is still incredibly convoluted. 

Additional problem came in: there is no way to tell inside MakeLocationSummary
whether we are compiling optimized or non-optimized version of the code, so I
can't request different number of temps (and I need different number of temps,
because in the non-optimized code I'd like to avoid specializing for a particual
type of the field). I am forced to perform some manual register assignment based
on my knowledge of how non-opt regalloc works. This is incredibly bad. 

However I don't see how to address this issue cleanly. I'd like to avoid passing
FlowGraphCompiler into locs()/MakeLocationSummary() because no other instruction
needs it. The only other option is to have it on the isolate. What do you think?

[Kevin Millikin (Google), 2013-03-15 12:17:21]

I've got a few comments, but otherwise this LGTM.  Keep in mind that I have not
reviewed the (raw)object parts in detail.

https://codereview.chromium.org/12529008/diff/19001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/12529008/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1528: static void
LoadValueCid(FlowGraphCompiler* compiler,
I think we have this pattern in a few places. EmitEqualityAsPolymorphicCall is
one.  In  StubCode::GenerateNArgsCheckInlineCacheStub we have almost the same
thing, but with a return in the middle an a pair of smi tag assembly code
sequences.  If we combined the returns we could have a single smi tag sequence
and it would fit the pattern too.

I'm not sure if it's worth it to combine, but it seems like it's getting
duplicated in a bunch of places now.

https://codereview.chromium.org/12529008/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1530: Register value_cid_reg) {
I guess the order of these registers should be swapped to fit our dst, src
ordering convention.

https://codereview.chromium.org/12529008/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1570: if (field_reg == kNoRegister) {
It seems to me that

(field_reg == kNoRegister) iff. (field_cid != kIllegalCid)

And by hypothesis

!compiler->is_optimizing() || (field_cid == kIllegalCid)

So instead of this condition you could write

!compiler->is_optimizing()

which makes the choice of EBX below less surprising.  Or assert it as below for
the value_cid_reg case.  Also, a comment.

https://codereview.chromium.org/12529008/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1616: // Field's guarded class id is
fixed by value's class id is not known.
by ==> but

https://codereview.chromium.org/12529008/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1662: __ Drop(2);  // Drop the field
and the value.
You're right, it's still spaghetti.  But now it's OUR kind of spaghetti.  :(

[Vyacheslav Egorov (Google), 2013-03-15 13:01:10]

Comments addressed.

Spaghetti are still there.

https://codereview.chromium.org/12529008/diff/19001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/12529008/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1528: static void
LoadValueCid(FlowGraphCompiler* compiler,
On 2013/03/15 12:17:21, kmillikin wrote:
> I think we have this pattern in a few places. EmitEqualityAsPolymorphicCall is
> one.  In  StubCode::GenerateNArgsCheckInlineCacheStub we have almost the same
> thing, but with a return in the middle an a pair of smi tag assembly code
> sequences.  If we combined the returns we could have a single smi tag sequence
> and it would fit the pattern too.
> 
> I'm not sure if it's worth it to combine, but it seems like it's getting
> duplicated in a bunch of places now.

Done.

https://codereview.chromium.org/12529008/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1530: Register value_cid_reg) {
On 2013/03/15 12:17:21, kmillikin wrote:
> I guess the order of these registers should be swapped to fit our dst, src
> ordering convention.

Done.

https://codereview.chromium.org/12529008/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1570: if (field_reg == kNoRegister) {
On 2013/03/15 12:17:21, kmillikin wrote:
> It seems to me that
> 
> (field_reg == kNoRegister) iff. (field_cid != kIllegalCid)
> 
> And by hypothesis
> 
> !compiler->is_optimizing() || (field_cid == kIllegalCid)
> 
> So instead of this condition you could write
> 
> !compiler->is_optimizing()
> 
> which makes the choice of EBX below less surprising.  Or assert it as below
for
> the value_cid_reg case.  Also, a comment.

Done.

https://codereview.chromium.org/12529008/diff/19001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1616: // Field's guarded class id is
fixed by value's class id is not known.
On 2013/03/15 12:17:21, kmillikin wrote:
> by ==> but

Done.

[Vyacheslav Egorov (Google), 2013-03-18 17:18:06]

Kevin, can you take another look please?

I updated type propagator to ensure that not-nullable information is correctly
propagated when phi types are recomputed.

[srdjan, 2013-03-18 18:54:35]

DBC

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1383: void DeoptimizeAt(const Code& optimized_code,
uword pc) {
ASSERT optimized code is optimized code?

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1744: const Object& value_cid =
Object::Handle(arguments.ArgAt(1));
s/value_cid/value/ ?

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:113: 
why the deleted line?

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:719: if ((*result)[j]->raw() == field->raw()) {
do "continue" here and  remove 'found' ?

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/flow_graph.h
File runtime/vm/flow_graph.h (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/flow_graph.h#n...
runtime/vm/flow_graph.h:147: ZoneGrowableArray<Field*>* FieldDependencies()
const;
Return const ZoneGrowableArray<..>& to document the fact that result cannot be
NULL.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1339: 
remove one line

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.h:263: class ConstrainedCompileType : public
ZoneCompileType {
Please add comments, documenting the two added classes.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:3233: if
(PcDescriptors::Handle(current_code.pc_descriptors()).Length() == 0) {
Please add comment what this is for.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/object.h#newco...
runtime/vm/object.h:1779: if (is_nullable() && guarded_cid() != kNullCid) {
Add ()

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/object.h#newco...
runtime/vm/object.h:1809: 
Remove one line.

[Vyacheslav Egorov (Google), 2013-03-18 19:41:18]

Comments addressed. Please take another look.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1383: void DeoptimizeAt(const Code& optimized_code,
uword pc) {
On 2013/03/18 18:54:35, srdjan wrote:
> ASSERT optimized code is optimized code?

Done.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1744: const Object& value_cid =
Object::Handle(arguments.ArgAt(1));
On 2013/03/18 18:54:35, srdjan wrote:
> s/value_cid/value/ ?

Done.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:113: 
On 2013/03/18 18:54:35, srdjan wrote:
> why the deleted line?

Done.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:719: if ((*result)[j]->raw() == field->raw()) {
On 2013/03/18 18:54:35, srdjan wrote:
> do "continue" here and  remove 'found' ?

C++ does not have continue with a label, so I can't continue the outer loop.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/flow_graph.h
File runtime/vm/flow_graph.h (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/flow_graph.h#n...
runtime/vm/flow_graph.h:147: ZoneGrowableArray<Field*>* FieldDependencies()
const;
On 2013/03/18 18:54:35, srdjan wrote:
> Return const ZoneGrowableArray<..>& to document the fact that result cannot be
> NULL.

Done.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1339: 
On 2013/03/18 18:54:35, srdjan wrote:
> remove one line

Done.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.h:263: class ConstrainedCompileType : public
ZoneCompileType {
On 2013/03/18 18:54:35, srdjan wrote:
> Please add comments, documenting the two added classes.

Done.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:3233: if
(PcDescriptors::Handle(current_code.pc_descriptors()).Length() == 0) {
On 2013/03/18 18:54:35, srdjan wrote:
> Please add comment what this is for.

Done.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/object.h#newco...
runtime/vm/object.h:1779: if (is_nullable() && guarded_cid() != kNullCid) {
On 2013/03/18 18:54:35, srdjan wrote:
> Add ()

Killed this accessor. It has confusing name (conflicting with normal accessor
guarded_cid) and was used in only one place.

https://codereview.chromium.org/12529008/diff/27001/runtime/vm/object.h#newco...
runtime/vm/object.h:1809: 
On 2013/03/18 18:54:35, srdjan wrote:
> Remove one line.

Done.

[Ivan Posva, 2013-03-19 04:42:05]

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1747:
field.UpdateCid(Class::Handle(value.clazz()).id());
Do you think it would be useful to have an ClassId() accessor on the Object
handle?

Then we would be able to write:
field.UpdateCid(value.ClassId());
instead of loading the class from the value and reading the id.

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/flow_graph.cc#...
runtime/vm/flow_graph.cc:731: return *result;
ditto.

This would morph into "return result;"

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/flow_graph.h
File runtime/vm/flow_graph.h (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/flow_graph.h#n...
runtime/vm/flow_graph.h:147: const ZoneGrowableArray<Field*>&
FieldDependencies() const;
Please do not return references as this makes the other side hard to read as
well, such as this:
guarded_fields = &flow_graph->FieldDependencies();

By returning a "const ZoneGrowableArray<Field*>*" you avoid that, no?

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:674: intptr_t is_nullable_;  // kNullCid if field can
contain null value and
As far as I understand this is a boolean value. Why not roll that into the
kind_bits_?

[Vyacheslav Egorov (Google), 2013-03-19 06:58:59]

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1747:
field.UpdateCid(Class::Handle(value.clazz()).id());
On 2013/03/19 04:42:05, Ivan Posva wrote:
> Do you think it would be useful to have an ClassId() accessor on the Object
> handle?
> 
> Then we would be able to write:
> field.UpdateCid(value.ClassId());
> instead of loading the class from the value and reading the id.

It can be useful but I would leave it out of scope for this change. This is slow
path anyways.

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/flow_graph.h
File runtime/vm/flow_graph.h (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/flow_graph.h#n...
runtime/vm/flow_graph.h:147: const ZoneGrowableArray<Field*>&
FieldDependencies() const;
On 2013/03/19 04:42:05, Ivan Posva wrote:
> Please do not return references as this makes the other side hard to read as
> well, such as this:
> guarded_fields = &flow_graph->FieldDependencies();
> 
> By returning a "const ZoneGrowableArray<Field*>*" you avoid that, no?

I actually agree that it makes caller harder to read. I was returning a pointer
but Srdjan suggested to return reference here to indicate that it is not NULL. 

I am fine returning either as there is a single call-site.

Should I switch back to the pointer?

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:674: intptr_t is_nullable_;  // kNullCid if field can
contain null value and
On 2013/03/19 04:42:05, Ivan Posva wrote:
> As far as I understand this is a boolean value. Why not roll that into the
> kind_bits_?

Having a boolean complicates checking and initialization in the generated code,
for the case when field does not have a feedback.

With a boolean I need to have additional control flow that decodes/encodes. With
cid here I just compare is_nullable_ to the cid of the value when comparison
with guarded_cid_ has failed. (and I just store value's cid when initializing it
initially). So the comment is somewhat incorrect. It can contain any other cid
if it is not null.

[Kevin Millikin (Google), 2013-03-19 11:40:55]

All LGTM now.

[srdjan, 2013-03-19 14:35:19]

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/flow_graph.h
File runtime/vm/flow_graph.h (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/flow_graph.h#n...
runtime/vm/flow_graph.h:147: const ZoneGrowableArray<Field*>&
FieldDependencies() const;
On 2013/03/19 06:58:59, Vyacheslav Egorov (Google) wrote:
> On 2013/03/19 04:42:05, Ivan Posva wrote:
> > Please do not return references as this makes the other side hard to read as
> > well, such as this:
> > guarded_fields = &flow_graph->FieldDependencies();
> > 
> > By returning a "const ZoneGrowableArray<Field*>*" you avoid that, no?
> 
> I actually agree that it makes caller harder to read. I was returning a
pointer
> but Srdjan suggested to return reference here to indicate that it is not NULL.

> 
> I am fine returning either as there is a single call-site.
> 
> Should I switch back to the pointer?

I see advantages of both ways. You pick.

[Ivan Posva, 2013-03-19 15:48:51]

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1747:
field.UpdateCid(Class::Handle(value.clazz()).id());
On 2013/03/19 06:58:59, Vyacheslav Egorov (Google) wrote:
> On 2013/03/19 04:42:05, Ivan Posva wrote:
> > Do you think it would be useful to have an ClassId() accessor on the Object
> > handle?
> > 
> > Then we would be able to write:
> > field.UpdateCid(value.ClassId());
> > instead of loading the class from the value and reading the id.
> 
> It can be useful but I would leave it out of scope for this change. This is
slow
> path anyways.

I did not mean to imply to have this in this CL. I was just wondering whether it
would be helpful as I see the class id accessed at several places, either going
through the class as here or by exploding the friends of class RawObject.

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:674: intptr_t is_nullable_;  // kNullCid if field can
contain null value and
On 2013/03/19 06:58:59, Vyacheslav Egorov (Google) wrote:
> On 2013/03/19 04:42:05, Ivan Posva wrote:
> > As far as I understand this is a boolean value. Why not roll that into the
> > kind_bits_?
> 
> Having a boolean complicates checking and initialization in the generated
code,
> for the case when field does not have a feedback.
> 
> With a boolean I need to have additional control flow that decodes/encodes.
With
> cid here I just compare is_nullable_ to the cid of the value when comparison
> with guarded_cid_ has failed. (and I just store value's cid when initializing
it
> initially). So the comment is somewhat incorrect. It can contain any other cid
> if it is not null.

In that case please update the comment.

[Ivan Posva, 2013-03-19 16:00:48]

LGTM with comments about comments.

-Ivan

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:4759: if (!dependent.IsNull()) {
Please add a comment what is being done here. As far as I understand you are
searching for a cleared WeakProperty. Also I am not sure it is safe and going to
stay safe to reuse a previously cleared WeakProperty.

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:4774: weak_property.set_value(code);
Please explain why both key and value are set to code. Would key be enough?

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:4784: static bool ShouldDeoptimize(const Array&
dependent_code, const Code& code) {
This is a bit awkwardly named. What it really does is to check whether the code
depends on this field.

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:4823: WeakProperty& weak_property = WeakProperty::Handle();
Please add comment what this does. As far as I understand it switches all
functions with current dependent code to unoptimized.

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:4841: void Field::UpdateCid(intptr_t cid) const {
Comment about the two fields being updated separately but in sync.

[Vyacheslav Egorov (Google), 2013-03-19 19:57:50]

More comments added. 

Rebasing and landing.

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:4759: if (!dependent.IsNull()) {
On 2013/03/19 16:00:49, Ivan Posva wrote:
> Please add a comment what is being done here. As far as I understand you are
> searching for a cleared WeakProperty. Also I am not sure it is safe and going
to
> stay safe to reuse a previously cleared WeakProperty.

Right now it seems safe. Comment added.

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:4774: weak_property.set_value(code);
On 2013/03/19 16:00:49, Ivan Posva wrote:
> Please explain why both key and value are set to code. Would key be enough?

Yes, key would be enough. I just wanted to fill both properties. It's like
implementing Set on top of Map as m[value] = value;

Removed.

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:4784: static bool ShouldDeoptimize(const Array&
dependent_code, const Code& code) {
On 2013/03/19 16:00:49, Ivan Posva wrote:
> This is a bit awkwardly named. What it really does is to check whether the
code
> depends on this field.

Done.

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:4823: WeakProperty& weak_property = WeakProperty::Handle();
On 2013/03/19 16:00:49, Ivan Posva wrote:
> Please add comment what this does. As far as I understand it switches all
> functions with current dependent code to unoptimized.

Done.

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:4841: void Field::UpdateCid(intptr_t cid) const {
On 2013/03/19 16:00:49, Ivan Posva wrote:
> Comment about the two fields being updated separately but in sync.

There is a comment in the header file. I added more comments to the body of this
function to reflect what is happening under each branch.

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/12529008/diff/32003/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:674: intptr_t is_nullable_;  // kNullCid if field can
contain null value and
On 2013/03/19 15:48:51, Ivan Posva wrote:
> On 2013/03/19 06:58:59, Vyacheslav Egorov (Google) wrote:
> > On 2013/03/19 04:42:05, Ivan Posva wrote:
> > > As far as I understand this is a boolean value. Why not roll that into the
> > > kind_bits_?
> > 
> > Having a boolean complicates checking and initialization in the generated
> code,
> > for the case when field does not have a feedback.
> > 
> > With a boolean I need to have additional control flow that decodes/encodes.
> With
> > cid here I just compare is_nullable_ to the cid of the value when comparison
> > with guarded_cid_ has failed. (and I just store value's cid when
initializing
> it
> > initially). So the comment is somewhat incorrect. It can contain any other
cid
> > if it is not null.
> 
> In that case please update the comment.

Done.

[Vyacheslav Egorov (Google), 2013-03-19 20:15:45]

Committed patchset #10 manually as r20235 (presubmit successful).