Addressed review feedback, please take another look
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);
On 2013/03/06 14:47:28, Toon Verwaest wrote:
GetNonTransitioningStoreMode? The store mode itself is not
untransitioned right?
Done.
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1649
src/ic.cc:1649: GetReceiverMapsForStub(Handle<Code>(target()),
&target_receiver_maps);
On 2013/03/06 14:47:28, Toon Verwaest wrote:
Not directly related to this CL, but I think we can get rid of
GetReceiverMapsForStub, and just use
target()->FindAllMaps(&target_receiver_maps) instead.
Done.
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
On 2013/03/06 14:47:28, Toon Verwaest wrote:
an IC
Done.
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1663
src/ic.cc:1663:
Code::GetKeyedAccessStoreMode(target()->extra_ic_state());
On 2013/03/06 14:47:28, Toon Verwaest wrote:
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.
Done.
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1685
src/ic.cc:1685: if (store_mode == IsGrowStoreMode(store_mode)) {
On 2013/03/06 14:47:28, Toon Verwaest wrote:
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.
Done.
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1700
src/ic.cc:1700: Handle<Map> new_map = ComputeTransitionedMap(receiver,
store_mode);
On 2013/03/06 14:47:28, Toon Verwaest wrote:
Can we also call this transitioned_receiver_map, to be consistent with
code
above?
Done.
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1721
src/ic.cc:1721: if (old_store_mode != STORE_NO_TRANSITION) {
On 2013/03/06 14:47:28, Toon Verwaest wrote:
Meaning STORE_AND_GROW_NO_TRANSITION?
Well, this is currently correct, but for the next CL OOB and COW will
also != STORE_NO_TRANSITION
https://codereview.chromium.org/12390031/diff/1013/src/ic.cc#newcode1724
src/ic.cc:1724: } else if (store_mode != old_store_mode) {
As discussed offline, no change needed.
On 2013/03/06 14:47:28, Toon Verwaest wrote:
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()) {
On 2013/03/06 14:47:28, Toon Verwaest wrote:
Maybe add a comment stating that for JSArrays (including external
arrays),
negative indices are handled as properties, and thus don't fall under
OOB.
Done.
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)) {
On 2013/03/06 14:47:28, Toon Verwaest wrote:
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;
Done.
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));
On 2013/03/06 14:47:28, Toon Verwaest wrote:
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.
Done.
https://codereview.chromium.org/12390031/diff/1013/src/stub-cache.cc#newcode904
src/stub-cache.cc:904: StrictModeFlag strict_mode) {
On 2013/03/06 14:47:28, Toon Verwaest wrote:
I presume store_mode is
ASSERT(stub_kind == KeyedStoreIC::STORE_NO_TRANSITION ||
stub_kind == KeyedStoreIC::STORE_AND_GROW_NO_TRANSITION);
Done.
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.
