LGTM. Some small comments, mostly comments that need to be updated.
http://codereview.chromium.org/6366003/diff/8001/src/ia32/lithium-gap-resolver-ia32.cc File src/ia32/lithium-gap-resolver-ia32.cc (right): http://codereview.chromium.org/6366003/diff/8001/src/ia32/lithium-gap-resolver-ia32.cc#newcode35 src/ia32/lithium-gap-resolver-ia32.cc:35: : cgen_(owner), moves_(32), source_uses_(), destination_uses_(), I prefer treating the initializers like arguments in declarations (breaking one per line if they don't all fit on one line): : cgen_(owner), moves_(32), source_uses_(), // ... http://codereview.chromium.org/6366003/diff/8001/src/x64/lithium-codegen-x64.h File src/x64/lithium-codegen-x64.h (right): http://codereview.chromium.org/6366003/diff/8001/src/x64/lithium-codegen-x64.h#newcode44 src/x64/lithium-codegen-x64.h:44: class LGapNode; No need for this forward declaration anymore. http://codereview.chromium.org/6366003/diff/8001/src/x64/lithium-gap-resolver-x64.cc File src/x64/lithium-gap-resolver-x64.cc (right): http://codereview.chromium.org/6366003/diff/8001/src/x64/lithium-gap-resolver-x64.cc#newcode208 src/x64/lithium-gap-resolver-x64.cc:208: } I guess there should be a blank line after this one to match the other cases. http://codereview.chromium.org/6366003/diff/8001/src/x64/lithium-gap-resolver-x64.cc#newcode248 src/x64/lithium-gap-resolver-x64.cc:248: // Register-memory. Use a free register as a temp if possible. Do not Update the comment so it doesn't mention the spill on demand trick of IA32. Just say you'll use on kScratchRegister. http://codereview.chromium.org/6366003/diff/8001/src/x64/lithium-gap-resolver-x64.cc#newcode258 src/x64/lithium-gap-resolver-x64.cc:258: } else if (source->IsStackSlot() && destination->IsStackSlot()) { This case could be } else if ((source->IsStackSlot() && destination->IsStackSlot()) || (source->IsDoubleStackSlot() && destination->IsDoubleStackSlot())) { http://codereview.chromium.org/6366003/diff/8001/src/x64/lithium-gap-resolver-x64.cc#newcode276 src/x64/lithium-gap-resolver-x64.cc:276: if (other->IsDoubleRegister()) { I think this case (double register/double register) would be clearer if you had before this: } else if (source->IsDoubleRegister() && destination->IsDoubleRegister()) { ... } else if ... // double register/memory cases. http://codereview.chromium.org/6366003/diff/8001/src/x64/lithium-gap-resolver-x64.cc#newcode289 src/x64/lithium-gap-resolver-x64.cc:289: // Double-width memory-to-memory. Spill on demand to use a general If you don't move this to share the other memory/memory case, update the comment here because you don't spill on demand. http://codereview.chromium.org/6366003/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
