Expand the instruction visitor to visit computations and values.

Expand the instruction visitor to visit computations and values.

For compilation passes we will want a visitor that can reach computations
and values, rather than virtual functions in the computation/value classes.
Implement the visitor signature and double dispatching.  Use it to remove
the virtual Print functions.

R=srdjan@google.com
BUG=
TEST=

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

[Kevin Millikin (Google), 2012-02-27 11:52:27]

It is ambiguous whether we are visiting a Value as a Value or as a Computation. 
I have decided that it is as a Computation (by convention)---so I've kept the
type-testing FlowGraphCompiler::LoadValue function for 'visiting' a Value as a
Value.

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

http://codereview.chromium.org/9471010/diff/1/runtime/vm/flow_graph_builder.h...
runtime/vm/flow_graph_builder.h:146: value_ = new TempVal(AllocateTempIndex());
I'm not in love with the abbreviated name, but it is at least consistent with
XxxComp and XxxInstr.

[srdjan, 2012-02-27 22:53:10]

LGTM with questions and comments

http://codereview.chromium.org/9471010/diff/3001/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.h (right):

http://codereview.chromium.org/9471010/diff/3001/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:3: // BSD-style license that can be found in
the LICENSE file.
Maybe now it is a good time to split Computations and Instructions in two
different header files?

http://codereview.chromium.org/9471010/diff/3001/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:100: intptr_t index_;
const ?

http://codereview.chromium.org/9471010/diff/3001/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:121: 
What additional value classes do you expect to come into the hierarchy? IMO, the
macros would be overkill for only two classes.

http://codereview.chromium.org/9471010/diff/3001/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:146: : name_(name), arguments_(arguments) { }
ASSERT(!arguments->IsEmpty())  // must have at least receiver.

http://codereview.chromium.org/9471010/diff/3001/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:479: class FlowGraphVisitor {
Is this a ValueObject or ZoneAllocated?
What is the advantage of merging Instruction and Computation visitors into one?

Please add comment describing what this class does.

[Kevin Millikin (Google), 2012-02-28 08:54:23]

http://codereview.chromium.org/9471010/diff/3001/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.h (right):

http://codereview.chromium.org/9471010/diff/3001/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:3: // BSD-style license that can be found in
the LICENSE file.
On 2012/02/27 22:53:10, srdjan wrote:
> Maybe now it is a good time to split Computations and Instructions in two
> different header files?

Sure, it's getting big enough.  I hesitated because of the naming issue: we
already have instructions.{h,cc}.

Instruction is such a juicy name for the IL instructions, but it makes sense for
machine instructions too.

Do you have any suggestions?

http://codereview.chromium.org/9471010/diff/3001/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:100: intptr_t index_;
On 2012/02/27 22:53:10, srdjan wrote:
> const ?

Yes, done.

http://codereview.chromium.org/9471010/diff/3001/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:121: 
On 2012/02/27 22:53:10, srdjan wrote:
> What additional value classes do you expect to come into the hierarchy? IMO,
the
> macros would be overkill for only two classes.

Definitely SSA phi values.  Possibly stack and context variable values, which
can be treated as values if the variables are definitely not assigned between a
definition and use.

I agree the macros look like overkill, and I won't complain if we decide to
remove them.  For now, I wanted a clear identification of what changes when we
add a new class.

http://codereview.chromium.org/9471010/diff/3001/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:146: : name_(name), arguments_(arguments) { }
On 2012/02/27 22:53:10, srdjan wrote:
> ASSERT(!arguments->IsEmpty())  // must have at least receiver.

OK.

http://codereview.chromium.org/9471010/diff/3001/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:479: class FlowGraphVisitor {
On 2012/02/27 22:53:10, srdjan wrote:
> Is this a ValueObject or ZoneAllocated?
> What is the advantage of merging Instruction and Computation visitors into
one?
> 
> Please add comment describing what this class does.

ValueObject base class added.

I don't think it makes sense to separate visiting of instruction and computation
at this point.  It seems like they're pretty tightly coupled  The instruction
visitor needs a computation visitor to drive it and there is some convenience
advantage to shared state in this case.

Comment added.