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?
http://codereview.chromium.org/6237002/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
