LGTM with nits.
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#newcode2128
src/code-stubs-hydrogen.cc:2128: Add<HCheckHeapObject>(receiver);
On 2014/12/03 11:48:30, mvstanton wrote:
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.
That's fine, but I didn't mean to imply you should do this. Using
HCheckHeapObject is fine. I was just ranting. Sorry if that confused you
:-)
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/03 11:48:30, mvstanton wrote:
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.
Acknowledged.
https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen.cc#newcode2091
src/code-stubs-hydrogen.cc:2091: IfBuilder if_correct_map(this);
nit: please don't shadow variables. (There's already an "if_correct_map"
builder in the outer scope.)
https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen.cc#newcode2098
src/code-stubs-hydrogen.cc:2098: TailCallMiss(receiver, name, slot,
vector, keyed_load);
nit: drop this...
https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen.cc#newcode2102
src/code-stubs-hydrogen.cc:2102: if_receiver_heap_object.Else();
...and this, and let all control flow paths fall through to a single
TailCallMiss() at the end.
https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen.cc#newcode2192
src/code-stubs-hydrogen.cc:2192: // Check if the IC in generic state.
nit: missing "is"
https://codereview.chromium.org/767743002/diff/60001/src/code-stubs-hydrogen.cc#newcode2204
src/code-stubs-hydrogen.cc:2204: // We never return here, it is a tail
call.
nit: move this comment after the call please.
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.
