Add forward iterator to iterate instructions inside a basic block.

Add forward iterator to iterate instructions inside a basic block.

It is currently only used in SSA renaming.
Committed: https://code.google.com/p/dart/source/detail?r=9438

[Kevin Millikin (Google), 2012-07-06 10:44:29]

https://chromiumcodereview.appspot.com/10692107/diff/1/vm/intermediate_langua...
File vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/10692107/diff/1/vm/intermediate_langua...
vm/intermediate_language.cc:530: Instruction*
Instruction::RemoveFromGraph(RemoveReturnValue ret) {
Do you think we could (should) get rid of this function an put it in the
iterator?  That way we would already know whether we wanted to return previous
or next, and we could avoid the linear search for the block entry when removing
the last instruction.

Disadvantage is that we need an iterator focused on the instruction to remove it
from the graph....

https://chromiumcodereview.appspot.com/10692107/diff/1/vm/intermediate_langua...
File vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10692107/diff/1/vm/intermediate_langua...
vm/intermediate_language.h:1949: class ForwardIterator : public ValueObject {
I think the class name is probably too generic.  We could call it
InstructionIterator and have it take a direction parameter.

[Florian Schneider, 2012-07-06 11:38:38]

Addressed comments. I used the iterator in two more places.

I'll add a backward iterator in a separate CL and use it in the liveness
analysis.

[Kevin Millikin (Google), 2012-07-06 11:58:42]

LGTM if you address or choose to ignore the small issue noted below.

https://chromiumcodereview.appspot.com/10692107/diff/6001/vm/il_printer.cc
File vm/il_printer.cc (right):

https://chromiumcodereview.appspot.com/10692107/diff/6001/vm/il_printer.cc#ne...
vm/il_printer.cc:43: ForwardInstructionIterator it(block_order_[i]);
I'm a bit uncomfortable with calling Current() on an iterator that is Done() --
it seems ill defined and relies on implementation details.  We could try:

Instruction* current = block_order_[i];
for (ForwardInstructionIterator it(current), !id.Done(), it.Advance()) {
  current = it.Current();
  // ...
}
if (current->successor() != NULL) {
  ASSERT(current->successor()->IsBlockEntry());
  OS::Print(" goto %d", current->successor()->AsBlockEntry()->block_id());
}

Is that equivalent?

https://chromiumcodereview.appspot.com/10692107/diff/6001/vm/intermediate_lan...
File vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10692107/diff/6001/vm/intermediate_lan...
vm/intermediate_language.h:1956: void RemoveCurrentFromGraph() {
I'd put this function in the .cc file.

[Florian Schneider, 2012-07-06 12:31:51]

https://chromiumcodereview.appspot.com/10692107/diff/6001/vm/il_printer.cc
File vm/il_printer.cc (right):

https://chromiumcodereview.appspot.com/10692107/diff/6001/vm/il_printer.cc#ne...
vm/il_printer.cc:43: ForwardInstructionIterator it(block_order_[i]);
On 2012/07/06 11:58:42, kmillikin wrote:
> I'm a bit uncomfortable with calling Current() on an iterator that is Done()
--
> it seems ill defined and relies on implementation details.  We could try:
> 
> Instruction* current = block_order_[i];
> for (ForwardInstructionIterator it(current), !id.Done(), it.Advance()) {
>   current = it.Current();
>   // ...
> }
> if (current->successor() != NULL) {
>   ASSERT(current->successor()->IsBlockEntry());
>   OS::Print(" goto %d", current->successor()->AsBlockEntry()->block_id());
> }
> 
> Is that equivalent?

Done.