Hi Jakob,
Thx for the comments! I've made two larger changes:
* logic change in the mono/poly case for the dispatcher. I was checking
array
length before it was necessary, because I know it will have 2 (load) or 3
(keyedload) elements no matter what.
* The confusing code in handlers has been improved. The special cases are
now
confined to Interceptor handler compilation.
Thanks for the look,
--Michael
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.
On 2014/12/02 08:43:06, Jakob wrote:
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.
Thanks, sounds like a good idea. I tried approximating it by just adding
Deopts after these tail calls, but it didn't reduce spill slots or their
usage. I'll tackle this in a follow-on CL.
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.
On 2014/12/02 08:43:06, Jakob wrote:
same here
Done.
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2059
src/code-stubs-hydrogen.cc:2059: IfBuilder mono_checker(this);
On 2014/12/02 08:43:06, Jakob wrote:
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.
Done.
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))
On 2014/12/02 08:43:06, Jakob wrote:
nit: Drop the static_cast, it's cleaner.
Done.
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))
On 2014/12/02 08:43:06, Jakob wrote:
nit: Drop the static_cast, it's cleaner.
Done. I reformulated this code a bit too, noticing that I don't need to
check the array length in the monomorphic case, but only if we didn't
find the map in the first tuple of the array.
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2120
src/code-stubs-hydrogen.cc:2120: // Are we monomorphic?
On 2014/12/02 08:43:06, Jakob wrote:
nit: s/Are we foo/Is the IC in foo state/, and please mention
polymorphic as
well.
Done.
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2124
src/code-stubs-hydrogen.cc:2124: array_checker.If<HCompareMap>(feedback,
On 2014/12/02 08:43:06, Jakob wrote:
This map check bakes in the assumption that the feedback type sentinel
is never
a Smi. Is that OK/intentional?
Yes indeed, that is why we have a pile of symbols to distinguish
different states. This convention was introduced for the CallIC last
summer to improve performance.
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2128
src/code-stubs-hydrogen.cc:2128: Add<HCheckHeapObject>(receiver);
On 2014/12/02 08:43:06, Jakob wrote:
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...
Oh yeah, creepy. Okay, I've turned the HCheckHeapObject into a
HCheckSmiAndBranch, handling the MISS in the else case.
https://codereview.chromium.org/767743002/diff/20001/src/code-stubs-hydrogen.cc#newcode2214
src/code-stubs-hydrogen.cc:2214: // in that case right?
On 2014/12/02 08:43:06, Jakob wrote:
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.
Wow...the overloaded megamorphic_stub() method in ic.h led me into this
error - I ended up providing megamorphic support for keyed loads :p. But
I want simple and basic correctness, so I'm withdrawing that support for
now. If it comes in handy I can bring it back in a follow-on CL.
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;
On 2014/12/02 08:43:07, Jakob wrote:
nit: for consistency, let's move both of these lines to the
initializer section.
(You can keep the DCHECK as it is.)
Done.
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 &&
On 2014/12/02 08:43:07, Jakob wrote:
Can we re-use IC::ICUseVector here and below?
Done.
https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler.cc#newcode146
src/ic/handler-compiler.cc:146: // miss occurs
On 2014/12/02 08:43:07, Jakob wrote:
nit: Trailing full stop.
Done.
https://codereview.chromium.org/767743002/diff/20001/src/ic/handler-compiler.cc#newcode150
src/ic/handler-compiler.cc:150: !reg.is(object_reg)) {
On 2014/12/02 08:43:07, Jakob wrote:
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.
Okay, I've refactored this. The problem is that Frontend() is usually
used in a simple way, with object_reg == receiver(). But the interceptor
case plays some tricks with it. I removed all these contortions here in
favor of confining them to interceptor handler compilation, where
they've been reformulated to make more sense.
The general idea is that the interceptor wants to store the holder in
the vector register, but MISS can still occur during that period. So
code that maintains the vector and slot may need to shift where it
stores them to scratch2() and scratch3() during that time.
This story can now be read in a straight line as a part of interceptor
handler compilation.
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
On 2014/12/02 08:43:07, Jakob wrote:
nit: Capitalization and punctuation, please.
Done.
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)) {
On 2014/12/02 08:43:07, Jakob wrote:
Again, can we re-use IC::ICUseVector() instead of duplicating the
kinds list?
Done.
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());
On 2014/12/02 08:43:07, Jakob wrote:
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)?
My earlier lengthy comment and the code changes answer these good
questions.
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.
