LGTM.
This is much better than the old code. Please also check if we hit all the
cases
in EmitMove and EmitSwap in our tests.
http://codereview.chromium.org/6263005/diff/2001/src/ia32/lithium-gap-resolver-ia32.cc
File src/ia32/lithium-gap-resolver-ia32.cc (right):
http://codereview.chromium.org/6263005/diff/2001/src/ia32/lithium-gap-resolver-ia32.cc#newcode64
src/ia32/lithium-gap-resolver-ia32.cc:64: if (!moves_[i].IsEliminated())
{
Not sure: can a move with a constant source operand be eliminated by
PerformMove at all?
http://codereview.chromium.org/6263005/diff/2001/src/ia32/lithium-gap-resolver-ia32.cc#newcode87
src/ia32/lithium-gap-resolver-ia32.cc:87: }
How about a slot assert that verifies the move-graph property that no
operand is used as a destination more than once?
http://codereview.chromium.org/6263005/diff/2001/src/ia32/lithium-gap-resolver-ia32.cc#newcode367
src/ia32/lithium-gap-resolver-ia32.cc:367: // spilling src at this
point. Memory-to-memory swaps are a rare case.
The comment mentions mem-to-mem swaps which are not handled here, but
further below.
http://codereview.chromium.org/6263005/diff/6001/src/ia32/lithium-gap-resolver-ia32.cc#newcode84
src/ia32/lithium-gap-resolver-ia32.cc:84: for (int i = moves->length() -
1; i >= 0; --i) {
Would it make the code more concise by passing a LMoveOperands* to
PerformMove/RemoveMove/...?
http://codereview.chromium.org/6263005/diff/6001/src/ia32/lithium-gap-resolver-ia32.cc#newcode92
src/ia32/lithium-gap-resolver-ia32.cc:92: // Each call to this function
performs a move and deletes it from the move
Can we ASSSERT(!moves_[index].IsPending()) here?
I think this is already covered by the !IsRedundant(), but
the predicates are a little confusing.
http://codereview.chromium.org/6263005/diff/6001/src/ia32/lithium-gap-resolver-ia32.cc#newcode104
src/ia32/lithium-gap-resolver-ia32.cc:104: LOperand* destination =
moves_[index].destination();
Reason for iterating in reverse order? Maybe add a comment about it.
http://codereview.chromium.org/6263005/diff/6001/src/ia32/lithium-gap-resolver-ia32.cc#newcode148
src/ia32/lithium-gap-resolver-ia32.cc:148: }
Maybe pass a LMoveOperands* here.
http://codereview.chromium.org/6263005/diff/6001/src/lithium-allocator.h
File src/lithium-allocator.h (right):
http://codereview.chromium.org/6263005/diff/6001/src/lithium-allocator.h#newcode359
src/lithium-allocator.h:359: bool IsEliminated() const { return source_
== NULL; }
maybe ASSERT(destination_ == NULL) here, or
return source_ == NULL && destination_ == NULL;
http://codereview.chromium.org/6263005/diff/6001/src/lithium.cc
File src/lithium.cc (right):
http://codereview.chromium.org/6263005/diff/6001/src/lithium.cc#newcode43
src/lithium.cc:43: for (int i = move_operands_.length() - 1; i >= 0;
--i) {
I'd also change the printint order to i=0..length-1.
http://codereview.chromium.org/6263005/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
