Looks good.
I would have suggested to land what you have, and then port to the other
platforms in another CL, but the platform-independent changes are making
that
difficult...
https://codereview.chromium.org/338963003/diff/30001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):
https://codereview.chromium.org/338963003/diff/30001/src/ia32/builtins-ia32.cc#newcode1016
src/ia32/builtins-ia32.cc:1016: // Update the index on the stack and in
register eax.
nit: s/eax/|key|/
https://codereview.chromium.org/338963003/diff/30001/src/ia32/builtins-ia32.cc#newcode1028
src/ia32/builtins-ia32.cc:1028: ASSERT(!key.is(eax));
Here, too, consider using Move() instead.
https://codereview.chromium.org/338963003/diff/30001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):
https://codereview.chromium.org/338963003/diff/30001/src/x64/builtins-x64.cc#newcode1100
src/x64/builtins-x64.cc:1100: ASSERT(!key.is(rax));
why does this matter? AFAICS violating the ASSERT wouldn't hurt, so we
can just skip it.
https://codereview.chromium.org/338963003/diff/30001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):
https://codereview.chromium.org/338963003/diff/30001/src/x64/full-codegen-x64.cc#newcode1847
src/x64/full-codegen-x64.cc:1847: // We need the receiver both on the
stack and in the accumulator.
s/accumulator/register/
https://codereview.chromium.org/338963003/diff/30001/src/x64/full-codegen-x64.cc#newcode2069
src/x64/full-codegen-x64.cc:2069: ASSERT(!load_receiver.is(rax));
suggestion: in all the cases where you're ASSERTing that we're not
moving unnecessarily, you could simply use Move() instead of
ASSERT+movp(). Move() does a src != dst check.
https://codereview.chromium.org/338963003/diff/30001/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):
https://codereview.chromium.org/338963003/diff/30001/src/x64/ic-x64.cc#newcode368
src/x64/ic-x64.cc:368: // receiver: receiver
Yay for high-entropy comments! :-P
AFAICS, the |receiver| register always contains the receiver, and the
|key| register always contains the key (it is only overwritten with the
"new" key in case of heapnumber->smi conversion). So you could drop the
comments about those two.
https://codereview.chromium.org/338963003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
