Generate KeyedLoadDictionaryElementStub with Hydrogen

Generate KeyedLoadDictionaryElementStub with Hydrogen

R=mvstanton@chromium.org

Committed: http://code.google.com/p/v8/source/detail?r=17804

[mvstanton, 2013-11-04 15:28:24]

Hi Danno, looks good. I have some nits, when those are addressed then...

lgtm.

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1323: current_probe + 1);
The recursive call is interesting here, but the base case is clear:
current_probe == kNumberDictionaryProbes, then deopt if that point is reached.
nice!

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1357: details_compare.End();
nit: I would engage in the { } indentation style here like BinaryOpStub and
FastNewClosureStub because it would be easier to parse the control flow.

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1370: USE(stub);
What are the USE statements for, can they be removed?

https://codereview.chromium.org/19492007/diff/160001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/19492007/diff/160001/src/hydrogen.cc#newcode1918
src/hydrogen.cc:1918: HValue* HGraphBuilder::BuildElementIndexHash(HValue*
index) {
Could you update the comment in macro-assembler-X.cc where it sez:

// Compute the hash code from the untagged key.  This must be kept in sync
// with ComputeIntegerHash in utils.h.

Add this location as another place to be kept in sync.

[Jakob Kummerow, 2013-11-06 10:48:11]

LGTM with a few more nits.

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1270: explicit CodeStubGraphBuilder(Isolate* isolate,
nit: no "explicit" necessary

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1289: HValue*
CodeStubGraphBuilder<KeyedLoadDictionaryElementStub>::
nit: I'd break after "HValue*" instead of after "::", but I leave that up to
you.

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1302: ? hash
nit: 4 space indent

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1326: result = graph()->GetConstantUndefined();
As discussed, the automatic environment padding handles this, but I'd weakly
prefer Push()ing some value (undefined, 0, take your pick) explicitly.

[danno, 2013-11-15 17:53:48]

Committed patchset #15 manually as r17804 (presubmit successful).

[danno, 2013-11-15 17:54:09]

Feedback addressed, landing

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1270: explicit CodeStubGraphBuilder(Isolate* isolate,
On 2013/11/06 10:48:12, Jakob wrote:
> nit: no "explicit" necessary

Done.

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1289: HValue*
CodeStubGraphBuilder<KeyedLoadDictionaryElementStub>::
On 2013/11/06 10:48:12, Jakob wrote:
> nit: I'd break after "HValue*" instead of after "::", but I leave that up to
> you.

Done.

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1302: ? hash
On 2013/11/06 10:48:12, Jakob wrote:
> nit: 4 space indent

Done.

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1323: current_probe + 1);
On 2013/11/04 15:28:24, mvstanton wrote:
> The recursive call is interesting here, but the base case is clear:
> current_probe == kNumberDictionaryProbes, then deopt if that point is reached.
> nice!

Done.

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1326: result = graph()->GetConstantUndefined();
On 2013/11/06 10:48:12, Jakob wrote:
> As discussed, the automatic environment padding handles this, but I'd weakly
> prefer Push()ing some value (undefined, 0, take your pick) explicitly.

Done.

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1357: details_compare.End();
On 2013/11/04 15:28:24, mvstanton wrote:
> nit: I would engage in the { } indentation style here like BinaryOpStub and
> FastNewClosureStub because it would be easier to parse the control flow.

Done.

https://codereview.chromium.org/19492007/diff/160001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:1370: USE(stub);
On 2013/11/04 15:28:24, mvstanton wrote:
> What are the USE statements for, can they be removed?

Done.

https://codereview.chromium.org/19492007/diff/160001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/19492007/diff/160001/src/hydrogen.cc#newcode1918
src/hydrogen.cc:1918: HValue* HGraphBuilder::BuildElementIndexHash(HValue*
index) {
On 2013/11/04 15:28:24, mvstanton wrote:
> Could you update the comment in macro-assembler-X.cc where it sez:
> 
> // Compute the hash code from the untagged key.  This must be kept in sync
> // with ComputeIntegerHash in utils.h.
> 
> Add this location as another place to be kept in sync.

Done.