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()) { On 2011/01/17 09:44:16, fschneider wrote:
Not sure: can a move with a constant source operand be eliminated by
PerformMove
at all?
Only if it's the move passed to PerformMove. http://codereview.chromium.org/6263005/diff/2001/src/ia32/lithium-gap-resolver-ia32.cc#newcode87 src/ia32/lithium-gap-resolver-ia32.cc:87: } On 2011/01/17 09:44:16, fschneider wrote:
How about a slot assert that verifies the move-graph property that no
operand is
used as a destination more than once?
Good idea. I will add it. 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. On 2011/01/17 09:44:16, fschneider wrote:
The comment mentions mem-to-mem swaps which are not handled here, but
further
below.
I will fix it. 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) { On 2011/01/17 09:44:16, fschneider wrote:
Would it make the code more concise by passing a LMoveOperands* to PerformMove/RemoveMove/...?
A little, but I'm nervous about passing addresses of elements in a List. Adding to the list can reallocate the backing store. Of course, here the algorithm doesn't necessarily work if we add to the list after we start performing moves, but I'd still like to be careful. If nothing else, the index makes it clear that the list is holding the pair and passing a pointer obscures that a bit. 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 On 2011/01/17 09:44:16, fschneider wrote:
Can we ASSSERT(!moves_[index].IsPending()) here?
I think this is already covered by the !IsRedundant(), but the predicates are a little confusing.
I agree the predicates are confusing (one has to read their implementation). That's probably unavoidable while keeping their names relatively short. I will assert !IsPending() && !IsRedundant() (the assert will hit a null dereference otherwise, which still catches the case but is a little less direct. 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(); On 2011/01/17 09:44:16, fschneider wrote:
Reason for iterating in reverse order? Maybe add a comment about it.
No reason, order is abitrary. I'll just stick with forward so nobody has to wonder 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: } On 2011/01/17 09:44:16, fschneider wrote:
Maybe pass a LMoveOperands* here.
I'd rather not. We don't need to mutate it and it's another pointer into a List. 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; } On 2011/01/17 09:44:16, fschneider wrote:
maybe ASSERT(destination_ == NULL) here, or
return source_ == NULL && destination_ == NULL;
I'll assert (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) { On 2011/01/17 09:44:16, fschneider wrote:
I'd also change the printint order to i=0..length-1.
I have no idea why it's backwards. I'll change it. http://codereview.chromium.org/6263005/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
