Hydrogen: fix keyed loads with string keys

Hydrogen: fix keyed loads with string keys

Keyed loads should not unconditionally be compiled to element loads. Update KeyedLoadICs to keep track of the key type, so that Hydrogen can emit ICs for string-keyed loads it doesn't have inline support for.

BUG=v8:3167
R=jkummerow@chromium.org

Committed: https://crrev.com/f6e68d2c2c1e92c14f220223f4a0b54b54aadff6
Cr-Commit-Position: refs/heads/master@{#25817}

[indutny, 2014-12-03 07:59:41]

On 2014/12/02 20:00:34, indutny wrote:

Actually, as pointed out by mraleph, it is not the best thing to do. I will try
to get it generate a generic load instruction instead.

Please ignore this issue, unless you have some tips for me ;)

[indutny, 2014-12-03 15:48:09]

With lots of help from @mraleph, I was able to figure out the better way to do
this.

This patch is something that I would like to submit, please take a look.

[danno, 2014-12-03 15:53:33]

danno@chromium.org changed reviewers:
+ jkummerow@chromium.org, verwaest@chromium.org

[danno, 2014-12-03 15:53:33]

Adding the appropriate reviewers...

[indutny, 2014-12-05 12:54:30]

Ping? ;)

[indutny, 2014-12-10 08:16:52]

Sorry for pinging you again, but I am highly interested in outcome of this CL.

[Jakob Kummerow, 2014-12-10 08:41:50]

Yes, it's on my list.

I like the general idea, but I'm not so happy with some of the details. I
haven't had enough time yet to formulate actionable comments.

[indutny, 2014-12-10 08:42:44]

Thank you, Jakob! Looking forward for your comments.

[Jakob Kummerow, 2014-12-10 15:21:18]

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#ne...
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#ne...
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#newcod...
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?

[indutny, 2014-12-10 18:08:54]

Jakob,

Thank you! Please find the updated patch with a proper commit message.

Hopefully, I haven't missed anything.

The only thing that bothers me a bit is:

```
Handle<Code> KeyedLoadIC::LoadElementStub(Handle<HeapObject> receiver)
```

You said that it could be kept the same as it is, but in fact the names of the
ic-compiler functions to do not suggest that the loads will be ELEMENT-only, so
I decided to explicitly pass the key type to them anyway.

[Jakob Kummerow, 2014-12-11 09:52:42]

The property IC compiler might have less-than-ideal naming conventions (from its
point of view, "keyed" == "elements"), but that's no reason to make it even more
confusing.

https://codereview.chromium.org/755513003/diff/40001/src/ic/ic-compiler.cc
File src/ic/ic-compiler.cc (right):

https://codereview.chromium.org/755513003/diff/40001/src/ic/ic-compiler.cc#ne...
src/ic/ic-compiler.cc:107: isolate->factory()->empty_string(), ELEMENT);
See how it says ELEMENT here? This method is only used to compile ELEMENT access
ICs, and that's the only thing it *can* handle, so there's really no need to
pretend that it's more flexible than that. Let's not add unnecessary complexity.

https://codereview.chromium.org/755513003/diff/40001/src/ic/ic-compiler.cc#ne...
src/ic/ic-compiler.cc:288: key_type);
So here you're replacing the hard-coded ELEMENT with the supposedly variable
key_type, but at the only call site of this function, ELEMENT is always passed
in. Again, let's not add complexity that's not required.

https://codereview.chromium.org/755513003/diff/40001/src/ic/ic-state.h
File src/ic/ic-state.h (right):

https://codereview.chromium.org/755513003/diff/40001/src/ic/ic-state.h#newcod...
src/ic/ic-state.h:239: 
nit: no need to add these empty lines

https://codereview.chromium.org/755513003/diff/40001/src/ic/ic.h
File src/ic/ic.h (right):

https://codereview.chromium.org/755513003/diff/40001/src/ic/ic.h#newcode438
src/ic/ic.h:438: // ExtraICState bits
nit: duplicate comment, drop it.

https://codereview.chromium.org/755513003/diff/40001/test/mjsunit/generic-mon...
File test/mjsunit/generic-monorphic-keyed-load.js (right):

https://codereview.chromium.org/755513003/diff/40001/test/mjsunit/generic-mon...
test/mjsunit/generic-monorphic-keyed-load.js:44: // generate KeyedLoadGeneric,
instead of element access and deopt check
nit: missing punctuation at the end.
Or you could just drop the comment. The test case doesn't really care what code
is generated.

https://codereview.chromium.org/755513003/diff/40001/test/mjsunit/generic-mon...
test/mjsunit/generic-monorphic-keyed-load.js:48: // Should not deoptimize
nit: same here. Again, just drop it. It's obvious what the next line does.

https://codereview.chromium.org/755513003/diff/40001/test/mjsunit/generic-mon...
test/mjsunit/generic-monorphic-keyed-load.js:49:
assertTrue(%GetOptimizationStatus(get) != 2);
Use "assertOptimized(get);" here.

[indutny, 2014-12-12 08:16:11]

Jakob,

Thank you! Should be better now.

[Jakob Kummerow, 2014-12-12 10:14:27]

Getting there.

I just noticed the test's name: "monorphic" is not a word, and even if you
correct the typo, "generic-monomorphic" is contradictory and confusing.
Suggestion: "keyed-load-with-string-key.js"

https://codereview.chromium.org/755513003/diff/60001/src/ic/ic-compiler.cc
File src/ic/ic-compiler.cc (right):

https://codereview.chromium.org/755513003/diff/60001/src/ic/ic-compiler.cc#ne...
src/ic/ic-compiler.cc:91: ExtraICState extra_ic_state) {
If you insist on keeping this additional argument, please explain to me in which
cases it changes anything. I still don't think we need it, and I don't like
adding extra code and extra parameters that don't do anything.
(Same below in the polymorphic case.)

[indutny, 2014-12-12 13:05:25]

Jakob,

Moved test and left comment.

[indutny, 2014-12-12 13:05:37]

https://codereview.chromium.org/755513003/diff/60001/src/ic/ic-compiler.cc
File src/ic/ic-compiler.cc (right):

https://codereview.chromium.org/755513003/diff/60001/src/ic/ic-compiler.cc#ne...
src/ic/ic-compiler.cc:91: ExtraICState extra_ic_state) {
On 2014/12/12 10:14:27, Jakob wrote:
> If you insist on keeping this additional argument, please explain to me in
which
> cases it changes anything. I still don't think we need it, and I don't like
> adding extra code and extra parameters that don't do anything.
> (Same below in the polymorphic case.)

I'd either need to pass this, or StrictMode to be able to generate
extra_ic_state here. Which do you prefer?

[Jakob Kummerow, 2014-12-12 13:34:17]

https://codereview.chromium.org/755513003/diff/60001/src/ic/ic-compiler.cc
File src/ic/ic-compiler.cc (right):

https://codereview.chromium.org/755513003/diff/60001/src/ic/ic-compiler.cc#ne...
src/ic/ic-compiler.cc:91: ExtraICState extra_ic_state) {
On 2014/12/12 13:05:37, indutny wrote:
> On 2014/12/12 10:14:27, Jakob wrote:
> > If you insist on keeping this additional argument, please explain to me in
> which
> > cases it changes anything. I still don't think we need it, and I don't like
> > adding extra code and extra parameters that don't do anything.
> > (Same below in the polymorphic case.)
> 
> I'd either need to pass this, or StrictMode to be able to generate
> extra_ic_state here. Which do you prefer?

Why do you need StrictMode? Loads don't depend on strict mode. We didn't need to
pass in anything before, and we don't want to change behavior for element loads
(or the way their stubs are compiled/cached/retrieved), and we also don't want
to toggle that bit arbitrarily: valid transitions are uninitialized -> ELEMENT
-> generic and uninitialized -> PROPERTY -> generic, but never ELEMENT ->
PROPERTY -> ELEMENT -> PROPERTY -> etc.

So why would we need to pass in anything now?

[indutny, 2014-12-12 13:46:03]

On 2014/12/12 13:34:17, Jakob wrote:
> https://codereview.chromium.org/755513003/diff/60001/src/ic/ic-compiler.cc
> File src/ic/ic-compiler.cc (right):
> 
>
https://codereview.chromium.org/755513003/diff/60001/src/ic/ic-compiler.cc#ne...
> src/ic/ic-compiler.cc:91: ExtraICState extra_ic_state) {
> On 2014/12/12 13:05:37, indutny wrote:
> > On 2014/12/12 10:14:27, Jakob wrote:
> > > If you insist on keeping this additional argument, please explain to me in
> > which
> > > cases it changes anything. I still don't think we need it, and I don't
like
> > > adding extra code and extra parameters that don't do anything.
> > > (Same below in the polymorphic case.)
> > 
> > I'd either need to pass this, or StrictMode to be able to generate
> > extra_ic_state here. Which do you prefer?
> 
> Why do you need StrictMode? Loads don't depend on strict mode. We didn't need
to
> pass in anything before, and we don't want to change behavior for element
loads

I meant ContextualMode, sorry. It is the one that is needed to construct
extra_state for this IC.

> (or the way their stubs are compiled/cached/retrieved), and we also don't want
> to toggle that bit arbitrarily: valid transitions are uninitialized -> ELEMENT
> -> generic and uninitialized -> PROPERTY -> generic, but never ELEMENT ->
> PROPERTY -> ELEMENT -> PROPERTY -> etc.

Hm... I understand the idea, but don't get what you want me to do in practice.
Should it just use not use extra_ic_state in these functions at all? This will
work because ELEMENT is equal to 0 at the moment, and that bit in extra state is
0 by default, but isn't it a bit incorrect?

> 
> So why would we need to pass in anything now?

[Jakob Kummerow, 2014-12-12 14:09:55]

> Hm... I understand the idea, but don't get what you want me to do in practice.

What I'm saying is: this code shouldn't change behavior, and doesn't need to be
modified, so don't modify it, especially not by making changes that don't do
anything. Just keep it the way it was.

> Should it just use not use extra_ic_state in these functions at all? This will
> work because ELEMENT is equal to 0 at the moment, and that bit in extra state
is
> 0 by default, but isn't it a bit incorrect?

It's perfectly correct. Feel free to add a STATIC_ASSERT(ELEMENT ==
kNoExtraICState) if you want to guard against future changes. However the most
likely future change is that all of this code will be ripped out and replaced by
vector ICs, which handle things differently (and much more cleanly) anyway.

[indutny, 2014-12-13 01:28:32]

Jakob,

Hopefully it is better now :) Please take another look.

[Jakob Kummerow, 2014-12-15 10:07:25]

OK, looks good now.

Please rebase; the patch doesn't apply to current HEAD. If you re-upload using a
proper V8 checkout (see https://code.google.com/p/v8-wiki/wiki/UsingGit), we can
let the CQ land this.

[indutny, 2014-12-15 12:51:22]

Jakob,

Took awhile, sorry!

It looks like it doesn't handle:

```
void TypeFeedbackOracle::KeyedPropertyReceiverTypes(
    FeedbackVectorICSlot slot, SmallMapList* receiver_types, bool* is_string) {
```

case in the latest master. Hopefully this is not a big problem.

[mvstanton, 2014-12-15 13:06:35]

mvstanton@chromium.org changed reviewers:
+ mvstanton@chromium.org

[mvstanton, 2014-12-15 13:06:36]

I'll follow-up with a small CL to make the Oracle learn this information from
the type feedback vector. I'll just wait for this one to land.

[Jakob Kummerow, 2014-12-15 13:07:57]

LGTM, let's see if the CQ can handle uploads from github checkouts.

https://codereview.chromium.org/755513003/diff/140001/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/755513003/diff/140001/src/typing.cc#newcode514
src/typing.cc:514: key_type = ELEMENT;
Yeah, actually this is a problem, because it means --vector-ics (which is the
future) will break this mechanism. But I've talked to mvstanton@ and he's
graciously offered to add support for this to the --vector-ics system.

[Jakob Kummerow, 2014-12-15 13:08:55]

The CQ bit was checked by jkummerow@chromium.org

[commit-bot: I haz the power, 2014-12-15 13:09:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/755513003/140001

[commit-bot: I haz the power, 2014-12-15 13:36:20]

Committed patchset #8 (id:140001)

[indutny, 2014-12-15 13:55:41]

On 2014/12/15 13:36:20, I haz the power (commit-bot) wrote:
> Committed patchset #8 (id:140001)

Yay, thank you everyone!

[commit-bot: I haz the power, 2014-12-17 10:13:19]

Patchset 8 (id:??) landed as
https://crrev.com/f6e68d2c2c1e92c14f220223f4a0b54b54aadff6
Cr-Commit-Position: refs/heads/master@{#25817}