Handle instance field and indexed increment operations.

Handle instance field and indexed increment operations.

Handle pre- and post-increment (decrement) in effect and non-effect
contexts.

R=srdjan@google.com
BUG=
TEST=

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

[Kevin Millikin (Google), 2012-03-07 13:44:54]

Messy because these are complicated operations, because of the assumptions on
temporaries by the naive code generator, and because writing such pattern
matching code is not great in C++.

By comparison, increment of local variables is a walk in the park.

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

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.h...
runtime/vm/flow_graph_builder.h:112: void BuildIncrOpIncrement(Token::Kind kind,
I suspect that there is a more general helper (for any binary op) lurking in
here.

[srdjan, 2012-03-07 19:12:52]

LGTM once bug fixed and comments addressed/answered.

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

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:322: ASSERT(for_receiver.value()->IsTemp());
This may not be always be true. A receiver can be a literal. Will add a test
case:


main() {
  var x = (const A()).foo++;
  Expect.equals(5, x);
}

class A {
  const A();
  set foo(value) {}
  get foo() { return 5; }
}

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:324: const int next_index =
for_receiver.temp_index();
ASSERT(next_index = receiver_index + 1)?

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:367: // Treat postincrement as if it were
preincrement.
.. because its results is not needed.

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:397: AddInstruction(new BindInstr(temp_index(),
new ConstantVal(placeholder)));
placeholder could also be Null object, e.g., Object::Handle().

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:423: //   t_n+1 <- ... index ...
I patched this CL and the code for IncrOpIndexed does not seem to work.

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:424: ArgumentGraphVisitor for_array(owner(),
temp_index());
Indexed access is not for arrays only. I would name it for_instance, or
for_receiver.

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:427: ASSERT(for_array.value()->IsTemp());
array or receiver could be a constant.

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:487: AddInstruction(new BindInstr(temp_index(),
new ConstantVal(placeholder)));
placeholder could be Object::Handle() (null object).

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:489: const int value_index =
BuildIncrOpIndexedLoad(node, temp_index() + 1);
The bug: loaded receiver will be store in the same temporary as the placeholder:

    t0 <-#0
    t0 <-LoadLocal(a)
    t1 <-#1
    t2 <- Pick(t0)
    t3 <- Pick(t1)
    t2 <-InstanceCall([], t2, t3)
....

[Kevin Millikin (Google), 2012-03-08 11:05:23]

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

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:322: ASSERT(for_receiver.value()->IsTemp());
On 2012/03/07 19:12:52, srdjan wrote:
> This may not be always be true. A receiver can be a literal. Will add a test
> case:
> 
> 
> main() {
>   var x = (const A()).foo++;
>   Expect.equals(5, x);
> }
> 
> class A {
>   const A();
>   set foo(value) {}
>   get foo() { return 5; }
> }

Yes, thanks.  The bug is that both uses are arguments, I should have
ArgumentGraphVisitor here, as in the indexed case.

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:324: const int next_index =
for_receiver.temp_index();
On 2012/03/07 19:12:52, srdjan wrote:
> ASSERT(next_index = receiver_index + 1)?

Done.  I got rid of all reliance on the return value from the recursive calls
since it is fixed and known (but assert that it is as expected).

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:367: // Treat postincrement as if it were
preincrement.
On 2012/03/07 19:12:52, srdjan wrote:
> .. because its results is not needed.

Done.

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:397: AddInstruction(new BindInstr(temp_index(),
new ConstantVal(placeholder)));
On 2012/03/07 19:12:52, srdjan wrote:
> placeholder could also be Null object, e.g., Object::Handle().

Do we have any preference?  It's never read except by the stack walker.

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:424: ArgumentGraphVisitor for_array(owner(),
temp_index());
On 2012/03/07 19:12:52, srdjan wrote:
> Indexed access is not for arrays only. I would name it for_instance, or
> for_receiver.

Changed to for_receiver (index and instance were too similar for my taste).

I just copied the 'array' name from the accessor IncrOpIndexedNode::array so the
name used locally here is a symptom of some other problem.

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:427: ASSERT(for_array.value()->IsTemp());
On 2012/03/07 19:12:52, srdjan wrote:
> array or receiver could be a constant.

ArgumentGraphVisitor should ensure it is named.

http://codereview.chromium.org/9622002/diff/1/runtime/vm/flow_graph_builder.c...
runtime/vm/flow_graph_builder.cc:489: const int value_index =
BuildIncrOpIndexedLoad(node, temp_index() + 1);
On 2012/03/07 19:12:52, srdjan wrote:
> The bug: loaded receiver will be store in the same temporary as the
placeholder:
> 
>     t0 <-#0
>     t0 <-LoadLocal(a)
>     t1 <-#1
>     t2 <- Pick(t0)
>     t3 <- Pick(t1)
>     t2 <-InstanceCall([], t2, t3)
> ....

Good catch and thanks for the test case.  Renaming failed due to programmer
error when the duplicated code was abstracted out....

[srdjan, 2012-03-08 17:46:08]

https://chromiumcodereview.appspot.com/9622002/diff/1/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://chromiumcodereview.appspot.com/9622002/diff/1/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:397: AddInstruction(new BindInstr(temp_index(),
new ConstantVal(placeholder)));
On 2012/03/08 11:05:23, kmillikin wrote:
> On 2012/03/07 19:12:52, srdjan wrote:
> > placeholder could also be Null object, e.g., Object::Handle().
> 
> Do we have any preference?  It's never read except by the stack walker.

The difference is only in the source code. To me it seems shorter and simpler to
write:

AddInstruction(new BindInstr(temp_index(), new ConstantVal(Object::Handle())));

However, it is no big deal, my intent was to make you aware of an alternative
way.