A first round of comments.
As you can see from the comments, I find the code in ic.cc and stub-cache.cc
quite difficult to navigate. Most of the time it's not immediately obvious
to me
what state we are in. At least some extra helper-functions / comments /
restructured tests are required.
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc
File src/ic.cc (right):
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1644
src/ic.cc:1644: store_mode = GetUntransitionedStoreMode(store_mode);
GetNonTransitioningStoreMode? The store mode itself is not
untransitioned right?
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1649
src/ic.cc:1649: GetReceiverMapsForStub(Handle<Code>(target()),
&target_receiver_maps);
Not directly related to this CL, but I think we can get rid of
GetReceiverMapsForStub, and just use
target()->FindAllMaps(&target_receiver_maps) instead.
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1658
src/ic.cc:1658: // There are several special cases where a IC that is
MONOMORPHIC can still
an IC
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1663
src/ic.cc:1663:
Code::GetKeyedAccessStoreMode(target()->extra_ic_state());
As far as I can tell, we are not certain that the target() is actually a
store-element stub. The map we retrieved may be a random map of a
prototype that we checked, in case of a prototype chain check required
for a, for example, global property load. We can make odd decisions
based on such maps. Probably this will not result in wrong semantics,
but may have weird performance implications.
Can we use the extra_ic_state somehow to distinguish different types of
Keyed Store ICs? Non-element-stores should only use the extra_ic_state
for encoding the strict-mode afaik.
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1685
src/ic.cc:1685: if (store_mode == IsGrowStoreMode(store_mode)) {
It seems like store_mode == IsGrowStoreMode(store_mode) is always false?
If you meant to write something like
IsNonTransitioningGrowStoreMode(store_mode), Isn't that a tautology
within its surrounding branch? If the original receiver maps were equal,
and we didn't transition (which is what the previous branch implies), we
can't have a transitioning store right?
In any case, this whole block of code is pretty hard to wade through.
I'd prefer if it were restructured to make it more obvious which cases
it's handling.
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1700
src/ic.cc:1700: Handle<Map> new_map = ComputeTransitionedMap(receiver,
store_mode);
Can we also call this transitioned_receiver_map, to be consistent with
code above?
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1721
src/ic.cc:1721: if (old_store_mode != STORE_NO_TRANSITION) {
Meaning STORE_AND_GROW_NO_TRANSITION?
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1724
src/ic.cc:1724: } else if (store_mode != old_store_mode) {
Can this happen? I presumed though would only be possible if
store_mode == STORE_NO_TRANSITION_IGNORE_OUT_OF_BOUNDS ||
store_mode == STORE_NO_TRANSITION_HANDLE_COW
(or old_store_mode == one of those).
But both cases are already excluded at the top of the function.
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1773
src/ic.cc:1773: if (receiver->IsJSArray()) {
Maybe add a comment stating that for JSArrays (including external
arrays), negative indices are handled as properties, and thus don't fall
under OOB.
https://codereview.chromium.org/12390031/diff/1013/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/12390031/diff/1013/src/objects.h#newcode208
src/objects.h:208: if (IsTransitionStoreMode(store_mode)) {
What about just
if (store_mode >= IGNORE_OUT_OF_BOUNDS) return store_mode;
if (store_mode >= STORE_AND_GROW) return STORE_AND_GROW_NO..;
return STORE_NO_TRANSITION;
https://codereview.chromium.org/12390031/diff/1013/src/stub-cache.cc
File src/stub-cache.cc (right):
https://codereview.chromium.org/12390031/diff/1013/src/stub-cache.cc#newcode440
src/stub-cache.cc:440: IsGrowStoreMode(store_mode));
It seems like we should not relax this condition. The stubs we compile
only care whether or not they are grow stubs. By allowing multiple
store_modes; which are encoded into the flags; we will have multiple
identical copies of code that are just different because of their
metadata.
As far as I can tell from the callsites, if my comment regarding "if
(*previous_receiver_map == receiver->map())" is correct at least, the
original condition should still hold.
https://codereview.chromium.org/12390031/diff/1013/src/stub-cache.cc#newcode904
src/stub-cache.cc:904: StrictModeFlag strict_mode) {
I presume store_mode is
ASSERT(stub_kind == KeyedStoreIC::STORE_NO_TRANSITION ||
stub_kind == KeyedStoreIC::STORE_AND_GROW_NO_TRANSITION);
https://codereview.chromium.org/12390031/
--
--
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/groups/opt_out.
