Support non-internalized string key lookups in Hydrogen KeyedLoadIC

Support non-internalized string key lookups in Hydrogen KeyedLoadIC

R=jkummerow@chromium.org

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

[Jakob Kummerow, 2014-06-27 15:37:51]

LGTM if you revert more unrelated changes

https://codereview.chromium.org/356213003/diff/40001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/356213003/diff/40001/src/hydrogen.cc#newcode1517
src/hydrogen.cc:1517: IfBuilder internalized(this);
nit: I think "not_internalized" would more accurately convey what it checks

https://codereview.chromium.org/356213003/diff/40001/src/hydrogen.cc#newcode1532
src/hydrogen.cc:1532: // Key guaranteed to be a unqiue string
nit: while you're here, "unique"

https://codereview.chromium.org/356213003/diff/40001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/356213003/diff/40001/src/runtime.cc#newcode1607
src/runtime.cc:1607: SealHandleScope shs(isolate);
no

https://codereview.chromium.org/356213003/diff/40001/src/runtime.h
File src/runtime.h (right):

https://codereview.chromium.org/356213003/diff/40001/src/runtime.h#newcode277
src/runtime.h:277: F(SetIteratorNext, 2, 1) \
no

[Jakob Kummerow, 2014-06-27 17:37:37]

https://codereview.chromium.org/356213003/diff/60001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/356213003/diff/60001/src/hydrogen.cc#newcode1521
src/hydrogen.cc:1521: not_internalized.Then();
Now you've changed both the name and the meaning. (Or am I confused?)
Either the IfBuilder should be called "internalized", then the condition should
be "not_internalized_bit == 0", and the Then() part should push the key while
the Else() part calls Runtime_InternalizeString.
Or it should be called not_internalized, with the condition being
"not_internalized != 0", the Then() branch calling Internalize and the Else()
branch pushing the original key (as the not_internalized bit is not unequal to
0... quadruple negation ftw!).

It would probably make sense to add a
RUNTIME_ASSERT(!StringShape(*string)->IsInternalized()) to
Runtime_InternalizeString...

[danno, 2014-06-27 17:43:28]

https://codereview.chromium.org/356213003/diff/40001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/356213003/diff/40001/src/hydrogen.cc#newcode1517
src/hydrogen.cc:1517: IfBuilder internalized(this);
On 2014/06/27 15:37:51, Jakob wrote:
> nit: I think "not_internalized" would more accurately convey what it checks

Done.

https://codereview.chromium.org/356213003/diff/40001/src/hydrogen.cc#newcode1532
src/hydrogen.cc:1532: // Key guaranteed to be a unqiue string
On 2014/06/27 15:37:51, Jakob wrote:
> nit: while you're here, "unique"

Done.

https://codereview.chromium.org/356213003/diff/40001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/356213003/diff/40001/src/runtime.cc#newcode1607
src/runtime.cc:1607: SealHandleScope shs(isolate);
On 2014/06/27 15:37:51, Jakob wrote:
> no

Done.

https://codereview.chromium.org/356213003/diff/40001/src/runtime.h
File src/runtime.h (right):

https://codereview.chromium.org/356213003/diff/40001/src/runtime.h#newcode277
src/runtime.h:277: F(SetIteratorNext, 2, 1) \
On 2014/06/27 15:37:51, Jakob wrote:
> no

Done.

https://codereview.chromium.org/356213003/diff/60001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/356213003/diff/60001/src/hydrogen.cc#newcode1521
src/hydrogen.cc:1521: not_internalized.Then();
On 2014/06/27 17:37:37, Jakob wrote:
> Now you've changed both the name and the meaning. (Or am I confused?)
> Either the IfBuilder should be called "internalized", then the condition
should
> be "not_internalized_bit == 0", and the Then() part should push the key while
> the Else() part calls Runtime_InternalizeString.
> Or it should be called not_internalized, with the condition being
> "not_internalized != 0", the Then() branch calling Internalize and the Else()
> branch pushing the original key (as the not_internalized bit is not unequal to
> 0... quadruple negation ftw!).
> 
> It would probably make sense to add a
> RUNTIME_ASSERT(!StringShape(*string)->IsInternalized()) to
> Runtime_InternalizeString...

This is intentional. I also switched from Token::NE to Token::EQ. This generates
better code, since the "then" case which doesn't have to do any work jumps over
the "else" case that does. But your right, the name is now flipped. I'll fix
that

As far as the runtime assert, it's not needed, since Factory does the right
thing with already internalized strings.

[danno, 2014-06-28 00:33:14]

Committed patchset #7 manually as r22070 (presubmit successful).