Store mode for keyed stores should be passed in from type feedback

Store mode for keyed stores should be passed in from type feedback
regardless of the map used in polymorphic stores.

BUG=
R=jkummerow@chromium.org, verwaest@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16323

[mvstanton, 2013-07-29 11:15:28]

Hi Toon, here is the small change we discussed Friday. I don't see why we
weren't already just passing store_mode(?).
thx,
--Michael

[mvstanton, 2013-07-30 07:14:51]

Hi Toon,
I find that in KeyedStoreIC::StoreElementStub() we take special care to ensure
that the store_mode of the IC is always updated in a way that respects the
totality of receiver maps seen thus far. If the IC is polymorphic, then the
store_mode has transition information stripped from it by passing through the
gate of GetNonTransitioningStoreMode(old_mode). Grow versus not-grow will be
preserved.

So I think the store_mode is safe to use without modification as we built up the
inlined polymorphic keyed store since it was good enough for each polymorphic
case in the IC, and adjusted carefully when new information arrived up to the
crankshaft point.
Thanks,
--Michael

[mvstanton, 2013-07-30 07:16:33]

Hit send too soon, that should have read:

"So I think the store_mode is safe to use without modification since it was good
enough for each polymorphic case in the IC, and adjusted carefully when new
information arrived up to the crankshaft point."

:),
--Michael

[Toon Verwaest, 2013-07-30 07:17:25]

lgtm

[Jakob Kummerow, 2013-07-31 09:43:00]

https://codereview.chromium.org/21058003/diff/10001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/21058003/diff/10001/src/hydrogen.cc#newcode5662
src/hydrogen.cc:5662: NULL,
I'm pretty sure that you need to pass |mapcompare| here. Otherwise the length
load can be hoisted past the dispatch, which can crash (when the code tries to
load a JSArray's length from an object that isn't a JSArray).

https://codereview.chromium.org/21058003/diff/10001/src/hydrogen.cc#newcode5674
src/hydrogen.cc:5674: ASSERT(IsExternalArrayElementsKind(elements_kind));
AFAICS BuildUncheckedMonomorphicElementAccess can handle external elements too.
So let's remove some more duplication :-)

if (IsDictionaryElementsKind(elements_kind)) {
  if (is_store) access = ... else ...
} else {
  ASSERT(IsFastElementsKind(...) || IsExternalArrayElementsKind(...));
  access = BuildUncheckedMonomorphicElementAccess(...);
}

https://codereview.chromium.org/21058003/diff/10001/test/mjsunit/array-store-...
File test/mjsunit/array-store-and-grow.js (right):

https://codereview.chromium.org/21058003/diff/10001/test/mjsunit/array-store-...
test/mjsunit/array-store-and-grow.js:192: // case.
nit: missing ')' after 'case'

https://codereview.chromium.org/21058003/diff/10001/test/mjsunit/array-store-...
test/mjsunit/array-store-and-grow.js:210: }());
nit: AFAIK the outer parentheses aren't even necessary, but when we do put them,
we usually put them around the function definition, as in:

(function() {...})();

https://codereview.chromium.org/21058003/diff/10001/test/mjsunit/array-store-...
test/mjsunit/array-store-and-grow.js:214: // deopt if you pass an element out of
bounds
nit: trailing full stop

[mvstanton, 2013-08-09 12:14:07]

Hi guys, finally got back to this.
Addressed comments. Perf looks neutral. Have a quick second look, thanks!
--Michael

https://codereview.chromium.org/21058003/diff/10001/test/mjsunit/array-store-...
File test/mjsunit/array-store-and-grow.js (right):

https://codereview.chromium.org/21058003/diff/10001/test/mjsunit/array-store-...
test/mjsunit/array-store-and-grow.js:192: // case.
On 2013/07/31 09:43:00, Jakob wrote:
> nit: missing ')' after 'case'

Done.

https://codereview.chromium.org/21058003/diff/10001/test/mjsunit/array-store-...
test/mjsunit/array-store-and-grow.js:210: }());
On 2013/07/31 09:43:00, Jakob wrote:
> nit: AFAIK the outer parentheses aren't even necessary, but when we do put
them,
> we usually put them around the function definition, as in:
> 
> (function() {...})();

Done.

https://codereview.chromium.org/21058003/diff/10001/test/mjsunit/array-store-...
test/mjsunit/array-store-and-grow.js:214: // deopt if you pass an element out of
bounds
On 2013/07/31 09:43:00, Jakob wrote:
> nit: trailing full stop

Done.

[Jakob Kummerow, 2013-08-21 16:46:49]

LGTM.

[Jakob Kummerow, 2013-08-26 12:24:51]

rebased version LGTM with nit.

https://codereview.chromium.org/21058003/diff/26001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/21058003/diff/26001/src/hydrogen.cc#newcode5707
src/hydrogen.cc:5707: // mapcompare is a checked object
nit: Capitalization and punctuation, please.

[mvstanton, 2013-08-26 12:27:50]

addressed nit, thx!

https://codereview.chromium.org/21058003/diff/26001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/21058003/diff/26001/src/hydrogen.cc#newcode5707
src/hydrogen.cc:5707: // mapcompare is a checked object
On 2013/08/26 12:24:51, Jakob wrote:
> nit: Capitalization and punctuation, please.

Done.

[mvstanton, 2013-08-26 12:28:20]

Committed patchset #8 manually as r16323 (presubmit successful).