Support instance getters and setters, indexed loads and stores.

Support instance getters and setters, indexed loads and stores.

These can be supported as instance calls.  In the case of instance
setters and indexed stores, there is a new computation type because of
the semantics of preserving the value.  For the example program:

void test(e) {
  print(e.forty_two);
  print(e.forty_two = 41);
}

we generate the graph:

 0: [target]
    t0 <-LoadLocal(e)
    t0 <-InstanceCall(get:forty_two, t0)
    StaticCall(print, t0)
    t0 <-#0
    t1 <-LoadLocal(e)
    t2 <-#41
    t0 <-InstanceSetter(t0, t1, t2)
    StaticCall(print, t0)
    return #null

and emit the correct code.

R=srdjan@google.com
BUG=
TEST=

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

[Kevin Millikin (Google), 2012-03-01 15:15:01]

This is built on top of https://chromiumcodereview.appspot.com/9564006/

I elected to implement these next because they used existing stuff but seemed a
bit warty too.

They require a pair of new computation types in the IL for instance setters and
indexed stores, to preserve the stored value.  I don't see any simpler way to
support it in the IL model.

Here I preallocate a temporary for the result (always) before evaluating the
operands rather than shuffling the operands after evaluating them as in the
existing code generator.

We can obviously avoid the preallocated temporary in effect contexts.  I'm not
sure if or how the optimizing compiler wants to see these, so I've kept it
simple.

[srdjan, 2012-03-01 23:19:31]

LGTM to make progress quickly.

I think a better solution is to introduce a temporary local for the rare
situation when the right hand side is needed. A temporary local can be allocated
before entering the graph builder, we would not need to add special nodes and
code. What do you think? Could your overwrite the corresponding visitor function
in ValueGraphVisitor so that we do not always try to preserve the value even for
effect?

Proposal:

0: [target]
t0 <-LoadLocal(e)
t0 <-InstanceCall(get:forty_two, t0)
StaticCall(print, t0)
t0 <- #41
StoreLocal(tmp, t0)
t0 <- LoadLocal(e)
t1 <- LoadLocal(tmp)
InstanceCall(set:forty_two, t0, t1)
t0 <- LoadLocal(tmp);
StaticCall(print, t0)
return #null

https://chromiumcodereview.appspot.com/9570015/diff/1/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://chromiumcodereview.appspot.com/9570015/diff/1/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_x64.cc:85: return true;
Can you check that last temp is on TOS, i.e., with highest index?

https://chromiumcodereview.appspot.com/9570015/diff/1/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_x64.cc:191: // computation, then call the getter.
Can you verify that place holder, value, array and and index are all in right
positions? (also in other added instructions).

[Kevin Millikin (Google), 2012-03-02 09:01:28]

We could also do what you suggest, but I'm not sure it's simpler.  It feels like
it's introducing a new concept (temporary local) to the optimizing compiler in
order to save a couple of computation types in the IL.  I'm not sure that's a
good tradeoff.

The optimizing compiler already has TempVal, which is exactly a temporary local.
 I claim (hand wavily) that "real" temporary locals would be not-quite-locals,
and not-quite-temporaries, and that we will regret them if introduced.

They seem like a (useful) hack to get around a system that can't allocate temps.
 But the optimizing backend will be able to do that, and the non-optimizing
backend can use push (like the current one does today).

These constructs are a bit warty.  Taking a step back, and looking at the
optimizing compiler, I think we want something like this for a setter or []=
operator whose value is used, (with e0.x = e1 as an example).  When we inline
the setter:

... code for e0 in t0 ...
... code for e1 in t1 ...
... inlined body uses t0 and t1 ...
... value of expression is t1 ...

Where t1 has (possibly) uses in the body as well as a use as the value of the
expression, and there is no placeholder at all.

And when we don't inline:

... code for e0 in t0 ...
... push t0 ...
... code for e1 in t1 ...
... push t1 ...
... call setter ...
... value of expression is t1 ...

Neither of which requires a placeholder.  So it's kind of annoying that we have
it at all, and it might be nice to find a way to make it completely local to the
setter call in the non-optimizing compiler.

https://chromiumcodereview.appspot.com/9570015/diff/1/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://chromiumcodereview.appspot.com/9570015/diff/1/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_x64.cc:85: return true;
On 2012/03/01 23:19:31, srdjan wrote:
> Can you check that last temp is on TOS, i.e., with highest index?

We can't easily check that it's actually on top of the stack.  That needs some
flow-sensitive stack height tracking that I've left as a TODO just above.  We
could also assert invariants about stack height (temporary indexes) in the graph
construction algorithm.

The current handling of calls/arguments is actually a bit fishy.  They mention
their arguments as operands, but they don't actually use them (for instance).

https://chromiumcodereview.appspot.com/9570015/diff/1/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_x64.cc:191: // computation, then call the getter.
On 2012/03/01 23:19:31, srdjan wrote:
> Can you verify that place holder, value, array and and index are all in right
> positions? (also in other added instructions).

Yes.