Remove the remaining LOperand-members from concrete LIR instructions....

Remove the remaining LOperand-members from concrete LIR instructions.

This change introduces the third template parameters for LIR instructions
to specify the number of temp-operands. This is one step towards
removing the instruction-summaries.

I also added hydrogen-accessors in more places and refactored
the LIR-branch instructions to have common super-classes to 
avoid code duplication.

Added MUST_USE_RESULT to the functions that record uses
and definitions so that all LOperands are stored in the
LIR instructions (and not only in the summaries).




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

[fschneider, 2011-01-12 22:25:19]

It's a pretty big CL - but there it concerns a lot of boilerplate code (and
removes a lot of it), so much of it is easy to review.

[Kevin Millikin (Chromium), 2011-01-13 08:45:59]

I've got just a few comments on this.

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

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.cc#newcode89
src/ia32/lithium-ia32.cc:89: stream->Add(" = ");
This adds a space before the '=' where before there was none.  Is that
intentional?

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (left):

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#oldcode343
src/ia32/lithium-ia32.h:343: T at(int i) const { return elems_[i]; }
This seems like a place that you could legitimately have:

T& operator[](int i) { return elems_[i]; }

and get rid of both at and set_at.

You could also assert that i is in bounds.

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

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode307
src/ia32/lithium-ia32.h:307: virtual void SetBranchTargets(int true_block_id,
int false_block_id) { }
Does this need to be on LInstruction?  I would expect an LControlInstruction
superclass of LUnaryControlInstruction and LBinaryControlInstruction where this
belongs (and is non-virtual).

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode372
src/ia32/lithium-ia32.h:372: int InputCount() const { return inputs_.length(); }
"return I" seems more direct.

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode376
src/ia32/lithium-ia32.h:376: int TempCount() const { return temps_.length(); }
"return T"?

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode383
src/ia32/lithium-ia32.h:383: private:
It also seems legitimate to make these protected so the subclasses can write:

inputs_[0] = input

rather than this->SetInputAt(0, input).

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode384
src/ia32/lithium-ia32.h:384: OperandContainer<LOperand*, R> outputs_;
Perhaps outputs_ should be results_ (or R should be O, which is worse because it
looks like 0) to keep up the symmetry.

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode529
src/ia32/lithium-ia32.h:529: template<int R, int T = 0>
We should strongly consider getting rid of classes LUnaryOperation and
LBinaryOperation. There doesn't seem much reason to use inheritance for the
number of inputs any more.

Here, we're not getting much functionality (aliases left() and right() for
inputs_[0] and inputs_[1], and a constructor so we can write
LBinaryOperation(left, right) instead of inputs_[0] = left; inputs_[1] = right;
in the subclasses.

I'd like to get rid of them because it's confusing to sometimes have one, two,
or three template parameters.  It might be more clear to always have <R,I> with
a default T and sometimes <R,I,T>.

[Vitaly Repeshko, 2011-01-13 11:44:07]

Drive by suggestion.

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

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode533
src/ia32/lithium-ia32.h:533: this->SetInputAt(0, left);
We could have a convenience function SetInputs(inputs...) defined for up to
certain number of input operands, so that in all instruction subclasses we could
write:
LSubclass(LOperand* op1, ..., LOperand* opN) {
  this->SetInputs(op1, ..., opN);
}
This could also be done for temps.

[fschneider, 2011-01-13 13:03:25]

Addressed comments.

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

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.cc#newcode89
src/ia32/lithium-ia32.cc:89: stream->Add(" = ");
On 2011/01/13 08:45:59, Kevin Millikin wrote:
> This adds a space before the '=' where before there was none.  Is that
> intentional?

Done.

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (left):

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#oldcode343
src/ia32/lithium-ia32.h:343: T at(int i) const { return elems_[i]; }
On 2011/01/13 08:45:59, Kevin Millikin wrote:
> This seems like a place that you could legitimately have:
> 
> T& operator[](int i) { return elems_[i]; }
> 
> and get rid of both at and set_at.
> 
> You could also assert that i is in bounds.

Done.

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

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode307
src/ia32/lithium-ia32.h:307: virtual void SetBranchTargets(int true_block_id,
int false_block_id) { }
On 2011/01/13 08:45:59, Kevin Millikin wrote:
> Does this need to be on LInstruction?  I would expect an LControlInstruction
> superclass of LUnaryControlInstruction and LBinaryControlInstruction where
this
> belongs (and is non-virtual).

The problem is that I can't cast to a LControlInstruction because it is a
template class.

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode372
src/ia32/lithium-ia32.h:372: int InputCount() const { return inputs_.length(); }
On 2011/01/13 08:45:59, Kevin Millikin wrote:
> "return I" seems more direct.

Done.

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode376
src/ia32/lithium-ia32.h:376: int TempCount() const { return temps_.length(); }
On 2011/01/13 08:45:59, Kevin Millikin wrote:
> "return T"?

Done.

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode383
src/ia32/lithium-ia32.h:383: private:
On 2011/01/13 08:45:59, Kevin Millikin wrote:
> It also seems legitimate to make these protected so the subclasses can write:
> 
> inputs_[0] = input
> 
> rather than this->SetInputAt(0, input).

Done.

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode384
src/ia32/lithium-ia32.h:384: OperandContainer<LOperand*, R> outputs_;
On 2011/01/13 08:45:59, Kevin Millikin wrote:
> Perhaps outputs_ should be results_ (or R should be O, which is worse because
it
> looks like 0) to keep up the symmetry.

Done.

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode529
src/ia32/lithium-ia32.h:529: template<int R, int T = 0>
On 2011/01/13 08:45:59, Kevin Millikin wrote:
> We should strongly consider getting rid of classes LUnaryOperation and
> LBinaryOperation. There doesn't seem much reason to use inheritance for the
> number of inputs any more.
> 
> Here, we're not getting much functionality (aliases left() and right() for
> inputs_[0] and inputs_[1], and a constructor so we can write
> LBinaryOperation(left, right) instead of inputs_[0] = left; inputs_[1] =
right;
> in the subclasses.
> 
> I'd like to get rid of them because it's confusing to sometimes have one, two,
> or three template parameters.  It might be more clear to always have <R,I>
with
> a default T and sometimes <R,I,T>.

Done.

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode533
src/ia32/lithium-ia32.h:533: this->SetInputAt(0, left);
On 2011/01/13 11:44:07, Vitaly wrote:
> We could have a convenience function SetInputs(inputs...) defined for up to
> certain number of input operands, so that in all instruction subclasses we
could
> write:
> LSubclass(LOperand* op1, ..., LOperand* opN) {
>   this->SetInputs(op1, ..., opN);
> }
> This could also be done for temps.

For this change I'd like to leave it more verbose - but I'm open for
improvements for the future.

I don't think I can make the number of parameters depend on a template
parameter, so these would need to be virtual? (which I want to avoid) - maybe a
macro would work?

[Vitaly Repeshko, 2011-01-13 13:06:51]

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

http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode533
src/ia32/lithium-ia32.h:533: this->SetInputAt(0, left);
On 2011/01/13 13:03:25, fschneider wrote:
> On 2011/01/13 11:44:07, Vitaly wrote:
> > We could have a convenience function SetInputs(inputs...) defined for up to
> > certain number of input operands, so that in all instruction subclasses we
> could
> > write:
> > LSubclass(LOperand* op1, ..., LOperand* opN) {
> >   this->SetInputs(op1, ..., opN);
> > }
> > This could also be done for temps.
> 
> For this change I'd like to leave it more verbose - but I'm open for
> improvements for the future.
> 
> I don't think I can make the number of parameters depend on a template
> parameter, so these would need to be virtual? (which I want to avoid) - maybe
a
> macro would work?

To implement this you define the function N times (for 1 to N arguments) and
rely on the nice property that unused parts of a template are not instantiated
and so their presence won't cause an error.

[fschneider, 2011-01-17 11:44:28]

On 2011/01/13 13:06:51, Vitaly wrote:
> http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h
> File src/ia32/lithium-ia32.h (right):
> 
>
http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode533
> src/ia32/lithium-ia32.h:533: this->SetInputAt(0, left);
> On 2011/01/13 13:03:25, fschneider wrote:
> > On 2011/01/13 11:44:07, Vitaly wrote:
> > > We could have a convenience function SetInputs(inputs...) defined for up
to
> > > certain number of input operands, so that in all instruction subclasses we
> > could
> > > write:
> > > LSubclass(LOperand* op1, ..., LOperand* opN) {
> > >   this->SetInputs(op1, ..., opN);
> > > }
> > > This could also be done for temps.
> > 
> > For this change I'd like to leave it more verbose - but I'm open for
> > improvements for the future.
> > 
> > I don't think I can make the number of parameters depend on a template
> > parameter, so these would need to be virtual? (which I want to avoid) -
maybe
> a
> > macro would work?
> 
> To implement this you define the function N times (for 1 to N arguments) and
> rely on the nice property that unused parts of a template are not instantiated
> and so their presence won't cause an error.

That would work. - but I'm not yet sure if we may get rid of constructors
completely in the future, so I rather wait with this. For now it's not that much
boilerplate code.