https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2036
src/code-stubs-hydrogen.cc:2036: // We never return here, it is a tail
call.
nit: I'd move this comment after the Add<...>.
If you feel you need to teach Hydrogen about the notion described in
this comment (e.g. because live ranges are an issue), the best approach
is probably modeled after HDeoptimize: end the block after the tail call
instruction, emit a goto to a new block, and update the
HMarkUnreachableBlocksPhase to mark that block as unreachable.
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2046
src/code-stubs-hydrogen.cc:2046: // We never return here, it is a tail
call.
same here
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2059
src/code-stubs-hydrogen.cc:2059: IfBuilder mono_checker(this);
Naming suggestion: if you name this guy "if_monomorphic", you'll get the
very natural "if_monomorphic.Then();" below. If you agree, consider
similar namings for other IfBuilders below.
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2075
src/code-stubs-hydrogen.cc:2075: ?
Add<HConstant>(static_cast<int32_t>(2))
nit: Drop the static_cast, it's cleaner.
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2120
src/code-stubs-hydrogen.cc:2120: // Are we monomorphic?
nit: s/Are we foo/Is the IC in foo state/, and please mention
polymorphic as well.
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2124
src/code-stubs-hydrogen.cc:2124: array_checker.If<HCompareMap>(feedback,
This map check bakes in the assumption that the feedback type sentinel
is never a Smi. Is that OK/intentional?
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2128
src/code-stubs-hydrogen.cc:2128: Add<HCheckHeapObject>(receiver);
Ugh. Hydrogen's ugly warts. HCheckHeapObject should be an iDef, and the
subsequent map load should have an explicit dependency on it. But that's
not this CL's business, and I guess by sheer luck the code just so
happens to be safe as it is...
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2214
src/code-stubs-hydrogen.cc:2214: // in that case right?
KeyedLoadIC currently doesn't support megamorphic state. For Smi keys,
it can be monomorphic, polymorphic, or generic. For non-Smi keys, it can
be monomorphic or generic. Yes, it's a mess. Future behavior TBD.
https://codereview.chromium.org/767743002/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/767743002/diff/20001/src/hydrogen-instructions.h#newcode2411
src/hydrogen-instructions.h:2411: call_mode_ = call_mode;
nit: for consistency, let's move both of these lines to the initializer
section. (You can keep the DCHECK as it is.)
https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler.cc
File src/ic/handler-compiler.cc (right):
https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler.cc#newcode135
src/ic/handler-compiler.cc:135: if (FLAG_vector_ics &&
Can we re-use IC::ICUseVector here and below?
https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler.cc#newcode146
src/ic/handler-compiler.cc:146: // miss occurs
nit: Trailing full stop.
https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler.cc#newcode150
src/ic/handler-compiler.cc:150: !reg.is(object_reg)) {
Uhm... you've pushed scratch2/scratch3 if object_reg == vector_reg. Now
you're popping them if object_reg != vector_reg. If that's intentional,
then it's confusing and deserves a comment.
https://codereview.chromium.org/767743002/diff/20001/src/ic/ia32/handler-compiler-ia32.cc
File src/ic/ia32/handler-compiler-ia32.cc (right):
https://codereview.chromium.org/767743002/diff/20001/src/ic/ia32/handler-compiler-ia32.cc#newcode81
src/ic/ia32/handler-compiler-ia32.cc:81: __ add(esp, Immediate(2 *
kPointerSize)); // remove vector and slot
nit: Capitalization and punctuation, please.
https://codereview.chromium.org/767743002/diff/20001/src/ic/ia32/handler-compiler-ia32.cc#newcode610
src/ic/ia32/handler-compiler-ia32.cc:610: (kind() == Code::LOAD_IC ||
kind() == Code::KEYED_LOAD_IC)) {
Again, can we re-use IC::ICUseVector() instead of duplicating the kinds
list?
https://codereview.chromium.org/767743002/diff/20001/src/ic/ia32/handler-compiler-ia32.cc#newcode613
src/ic/ia32/handler-compiler-ia32.cc:613: PushVectorAndSlot(scratch2(),
scratch3());
Why the case distinction? Pushing scratch registers isn't any faster
than pushing the real things, or is it? Do they contain useful values at
this point?
Also, it's not evident why slot and vector being on the stack is related
to holder and receiver registers being identical. Can you please add
that to the comment (if you keep the distinction)?
https://codereview.chromium.org/767743002/
--
--
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.
