Remove instruction summaries and provide a LIR-interface for the register all...

Remove instruction summaries.

Instead of constructing a temporary container for all LOperands of each
instruction, the register works directly on the LIR instructions that
 provide an abstract interface for input/output/temp operands.

This saves allocation of zone memory and speeds up LIR construction,
but makes iterating over all uses in the register allocator slightly
more expensive because environment uses are stored in a linked list of
environments. We can fix this by using a flat representation of LOperands.



Committed: http://code.google.com/p/v8/source/detail?r=6638

[Kevin Millikin (Chromium), 2011-01-20 12:20:30]

I've got lots of comments about the iterator.  The biggest one is I usually like
to expose Advance() and not have it rolled into Next().

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:88: for (UseIterator it = GetUseIterator();
it.HasNext(); ) {
I don't think the GetUseIterator/GetTempIterator functions add much.  You could
just use constructor syntax here:

for (UseIterator it(this); it.HasNext(); ) { ... }

You might also consider separating the functions for getting the current thing
and advancing to the next thing so you can write:

for (UseIterator it(this); it.HasNext(); it.Advance()) { ... }

so it reads better as a for loop (or keep HasNext's advancing semantics and
write it as a while loop?

{ UseIterator it(this);
  while (it.HasNext()) { ... }
}

).

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:945: chunk_->AddInstruction(instr, current_block_);
Since you now always ignore the return value, you can AddInstruction a void
function (and simplify its implementation a bit).

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:338: bool IsSaveDoubles() const { return
is_save_doubles_; }
These two names are not entirely consistent: is_thing_ vs. is_action_;
MarkAsThing vs. MarkAsAction; and IsMarkedAsThing vs. IsAction.

I usually suggest is_thing_/IsThing() and should_action_/ShouldAction().  And
IsMarkedAsThing() is just a longer way to say IsThing().

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:340: virtual bool HasResult() const  = 0;
const doesn't care if it has extra spaces around it, but I do!

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:392: static T t = NULL;
Do we really write NULL and not 0 to initialize a T?

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator-inl.h
File src/lithium-allocator-inl.h (right):

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator-inl.h...
src/lithium-allocator-inl.h:33: #if V8_TARGET_ARCH_IA32
Ultimately (not as part of this change), I'd like to have lithium.h include the
appropriate lithium-ia32.h.  There should be something one merely includes to
get the correct thing, without having to test at each point it wants to be
included.

Here I guess you actually want the lithium instructions, so fixing this site is
blocked by renaming ia32/lithium-ia32.h to ia32/lithium-instructions.h.  Then
you can have a platform independent "lithium-instructions.h" that just includes
the appropriate platform-specific one and here just write:

#include "lithium-instructions.h"

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator-inl.h...
src/lithium-allocator-inl.h:52: bool TempIterator::HasNext() { return current_ <
instr_->TempCount(); }
I'm not sure it makes much difference, but I'd put the limit
(instr_->TempCount()) in a field of the iterator.

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator-inl.h...
src/lithium-allocator-inl.h:92: env_iterator_(instr->HasEnvironment() ?
instr->environment() : NULL) { }
Using SetOncePointer for the environment is probably a mistake.  From
SetOnecePointer's declaration: "A pointer that can only be set once and doesn't
allow NULL values."  This one effectively does allow NULL values.

LInstruction::HasEnvironment is the bad smell.  We normally shouldn't have to
ask a SetOncePointer if it's been set.

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc
File src/lithium-allocator.cc (right):

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc#ne...
src/lithium-allocator.cc:779: for (TempIterator it = first->GetTempIterator();
it.HasNext(); ) {
for (TempIterator it(first); it.HasNext(); it.Advance()) { ... }

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc#ne...
src/lithium-allocator.cc:780: LUnallocated* temp = 
LUnallocated::cast(it.Next());
There's an extra space after =.

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc#ne...
src/lithium-allocator.cc:926: LInstruction* instr =
chunk_->instructions()->at(index);
It might be easier to read and write these sites if there was a simple
LChunk::InstructionAt(int) function.

http://codereview.chromium.org/6352006/diff/24001/src/lithium.h
File src/lithium.h (right):

http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode501
src/lithium.h:501: ASSERT(env_ != NULL);
In normal use, should one ever call Next on an environment that is not known to
HasNext()?

In other words, I would expect ASSERT(HasNext()) rather than checking for it.

http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode521
src/lithium.h:521: ShallowIterator shallow_iterator() { return
ShallowIterator(this); }
This function (and deep_iterator) probably shouldn't have hacker_style_naming. 
It returns a fresh object on each call so it's not just an accessor, and the
object has a non-trivial constructor.

http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode529
src/lithium.h:529: if (current_env_ == NULL) return false;
I think you can simplify this class's implementation a bit.  The current_env_ is
the environment where Next() will be found if HasNext() and NULL otherwise.  The
current_iterator_ is always a shallow iterator for the current environment.  The
shallow iterator for the NULL environment does not HasNext().

So:

inline bool HasNext() { return current_iterator_.HasNext(); }

http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode536
src/lithium.h:536: inline LOperand* Next() {
inline LOperand* Next() {
  ASSERT(current_iterator_.HasNext());
  LOperand* operand = current_iterator_.Next();
  Advance();
  return operand;
}

http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode544
src/lithium.h:544: inline void Advance() {
inline void Advance() {
  while (!current_iterator_.HasNext() && current_env_ != NULL) {
    current_env_ = current_env_->outer();
    current_iterator_ = ShallowIterator(current_env_);
  }
}

Then add to the constructor a call to Advance();

[fschneider, 2011-01-20 17:13:08]

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:88: for (UseIterator it = GetUseIterator();
it.HasNext(); ) {
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> I don't think the GetUseIterator/GetTempIterator functions add much.  You
could
> just use constructor syntax here:
> 
> for (UseIterator it(this); it.HasNext(); ) { ... }
> 
> You might also consider separating the functions for getting the current thing
> and advancing to the next thing so you can write:
> 
> for (UseIterator it(this); it.HasNext(); it.Advance()) { ... }
> 
> so it reads better as a for loop (or keep HasNext's advancing semantics and
> write it as a while loop?
> 
> { UseIterator it(this);
>   while (it.HasNext()) { ... }
> }
> 
> ).

Done.

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:945: chunk_->AddInstruction(instr, current_block_);
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> Since you now always ignore the return value, you can AddInstruction a void
> function (and simplify its implementation a bit).

Done.

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:338: bool IsSaveDoubles() const { return
is_save_doubles_; }
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> These two names are not entirely consistent: is_thing_ vs. is_action_;
> MarkAsThing vs. MarkAsAction; and IsMarkedAsThing vs. IsAction.
> 
> I usually suggest is_thing_/IsThing() and should_action_/ShouldAction().  And
> IsMarkedAsThing() is just a longer way to say IsThing().

IsCall is already taken as the name of the type-tester - i can make both
IsMarkedAs...

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:340: virtual bool HasResult() const  = 0;
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> const doesn't care if it has extra spaces around it, but I do!

Done.

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:392: static T t = NULL;
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> Do we really write NULL and not 0 to initialize a T?

Done.

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator-inl.h
File src/lithium-allocator-inl.h (right):

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator-inl.h...
src/lithium-allocator-inl.h:52: bool TempIterator::HasNext() { return current_ <
instr_->TempCount(); }
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> I'm not sure it makes much difference, but I'd put the limit
> (instr_->TempCount()) in a field of the iterator.

Good point. Done.

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator-inl.h...
src/lithium-allocator-inl.h:92: env_iterator_(instr->HasEnvironment() ?
instr->environment() : NULL) { }
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> Using SetOncePointer for the environment is probably a mistake.  From
> SetOnecePointer's declaration: "A pointer that can only be set once and
doesn't
> allow NULL values."  This one effectively does allow NULL values.
> 
> LInstruction::HasEnvironment is the bad smell.  We normally shouldn't have to
> ask a SetOncePointer if it's been set.

Done. Made it a normal pointer.

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc
File src/lithium-allocator.cc (right):

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc#ne...
src/lithium-allocator.cc:779: for (TempIterator it = first->GetTempIterator();
it.HasNext(); ) {
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> for (TempIterator it(first); it.HasNext(); it.Advance()) { ... }

Done.

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc#ne...
src/lithium-allocator.cc:780: LUnallocated* temp = 
LUnallocated::cast(it.Next());
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> There's an extra space after =.

Done.

http://codereview.chromium.org/6352006/diff/24001/src/lithium-allocator.cc#ne...
src/lithium-allocator.cc:926: LInstruction* instr =
chunk_->instructions()->at(index);
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> It might be easier to read and write these sites if there was a simple
> LChunk::InstructionAt(int) function.

Done. Introduces InstructionAt and GapAt helpers.

http://codereview.chromium.org/6352006/diff/24001/src/lithium.h
File src/lithium.h (right):

http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode501
src/lithium.h:501: ASSERT(env_ != NULL);
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> In normal use, should one ever call Next on an environment that is not known
to
> HasNext()?
> 
> In other words, I would expect ASSERT(HasNext()) rather than checking for it.

Done.

http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode521
src/lithium.h:521: ShallowIterator shallow_iterator() { return
ShallowIterator(this); }
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> This function (and deep_iterator) probably shouldn't have hacker_style_naming.

> It returns a fresh object on each call so it's not just an accessor, and the
> object has a non-trivial constructor.

Done.

http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode529
src/lithium.h:529: if (current_env_ == NULL) return false;
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> I think you can simplify this class's implementation a bit.  The current_env_
is
> the environment where Next() will be found if HasNext() and NULL otherwise. 
The
> current_iterator_ is always a shallow iterator for the current environment. 
The
> shallow iterator for the NULL environment does not HasNext().
> 
> So:
> 
> inline bool HasNext() { return current_iterator_.HasNext(); }

Done.

http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode536
src/lithium.h:536: inline LOperand* Next() {
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> inline LOperand* Next() {
>   ASSERT(current_iterator_.HasNext());
>   LOperand* operand = current_iterator_.Next();
>   Advance();
>   return operand;
> }

Done.

http://codereview.chromium.org/6352006/diff/24001/src/lithium.h#newcode544
src/lithium.h:544: inline void Advance() {
On 2011/01/20 12:20:30, Kevin Millikin wrote:
> inline void Advance() {
>   while (!current_iterator_.HasNext() && current_env_ != NULL) {
>     current_env_ = current_env_->outer();
>     current_iterator_ = ShallowIterator(current_env_);
>   }
> }
> 
> Then add to the constructor a call to Advance();

Done.

[Kevin Millikin (Chromium), 2011-02-04 11:48:09]

LGTM.

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/6352006/diff/24001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:945: chunk_->AddInstruction(instr, current_block_);
I mean "make AddInstruction a void function".