This can be simplified quite a bit. Take a look at
https://codereview.chromium.org/648703002. What that CL doesn't do, you
don't
need to do either.
Also, the commit description needs an update, as it still describes your
first
approach. The bug line should be just:
BUG=v8:3167
And how about adding a test case? You can take the one from the bug.
https://codereview.chromium.org/755513003/diff/20001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/755513003/diff/20001/src/ast.h#newcode1843
src/ast.h:1843: class KeyTypeField : public BitField16<IcCheckType, 3,
1> {};
This must be a BitField8!
https://codereview.chromium.org/755513003/diff/20001/src/ic/ic-compiler.cc
File src/ic/ic-compiler.cc (right):
https://codereview.chromium.org/755513003/diff/20001/src/ic/ic-compiler.cc#newcode67
src/ic/ic-compiler.cc:67: // Update key type
nit: punctuation (but see comment on next line).
https://codereview.chromium.org/755513003/diff/20001/src/ic/ic-compiler.cc#newcode68
src/ic/ic-compiler.cc:68: IcCheckType key_type = name->IsString() ?
PROPERTY : ELEMENT;
This distinction doesn't make sense, we only ever come here to compile
PROPERTY handlers.
Also, when name is not a string, then it's a symbol, and treating symbol
lookups as elements accesses is clearly wrong.
See the KEYED_STORE_IC path right above -- when we're here, it's always
a property access.
https://codereview.chromium.org/755513003/diff/20001/src/ic/ic-state.h
File src/ic/ic-state.h (right):
https://codereview.chromium.org/755513003/diff/20001/src/ic/ic-state.h#newcode241
src/ic/ic-state.h:241: class KeyedLoadICState FINAL BASE_EMBEDDED {
I see that you're just following an existing pattern here, but I'm not
convinced this logic needs to be double-wrapped. Two 1-liners would do
it, no?
Besides, KeyedLoadICs so far inherited the extra IC state from LoadICs.
I'm pretty sure that we want to preserve that. So again, look at how
KeyedStoreICs add to the bits they inherit from StoreIC, and model
KeyedLoadICs after that.
https://codereview.chromium.org/755513003/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (right):
https://codereview.chromium.org/755513003/diff/20001/src/ic/ic.cc#newcode1236
src/ic/ic.cc:1236: IcCheckType key_type = key->IsString() ? PROPERTY :
ELEMENT;
This should be key->IsName(). Then again the key is never a name here,
so this entire method doesn't need to be modified.
https://codereview.chromium.org/755513003/diff/20001/src/ic/ic.cc#newcode1306
src/ic/ic.cc:1306: stub = LoadElementStub(receiver, key);
Look at the conditions above. We only get here if key is a number and
fits in a smi.
https://codereview.chromium.org/755513003/diff/20001/src/ic/ic.h
File src/ic/ic.h (right):
https://codereview.chromium.org/755513003/diff/20001/src/ic/ic.h#newcode483
src/ic/ic.h:483: inline void set_target(Code* code);
Why do you need to override this? Can you explain why ContextualMode
doesn't matter for keyed loads?
https://codereview.chromium.org/755513003/
--
--
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.
