Refactor turning polymorphic loads into monomorphic loads

Refactor turning polymorphic loads into monomorphic loads
- remove special code for polymorphic loading of array length
- use it in VisitCountOperation

BUG=
R=hpayer@chromium.org

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

[Toon Verwaest, 2013-06-13 11:03:17]

PTAL

[Hannes Payer (out of office), 2013-06-13 15:26:36]

looking good, first round of comments

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode6319
src/hydrogen.cc:6319: HInstruction*
HOptimizedGraphBuilder::LoadPolymorphicAsMonomorphic(
Would it make more sense to name this method TryLoadPolymorphicAsMonomorphic?

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode6365
src/hydrogen.cc:6365: void
HOptimizedGraphBuilder::HandlePolymorphicLoadNamedField(Property* expr,
Can you move "Property* expr" to the next line?

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode6369
src/hydrogen.cc:6369: 
remove this newline

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode6677
src/hydrogen.cc:6677: if (monomorphic) {
can you restructure the code 
  monomorphic = false;
} 
if (monomorphic) {
...

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode6955
src/hydrogen.cc:6955: // OS::DebugBreak();
I guess you do not need that anymore.

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode9261
src/hydrogen.cc:9261: if (monomorphic) {
can you restructure the code 
  monomorphic = false;
} 
if (monomorphic) {
...

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode9272
src/hydrogen.cc:9272: if (load == NULL) load = BuildLoadNamedGeneric(object,
name, prop);
This sequence of code is exactly the same as in line 6666. I think it makes
sense to move it into a separate function. WDYT?

[Toon Verwaest, 2013-06-13 16:33:58]

Addressed comments

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode6319
src/hydrogen.cc:6319: HInstruction*
HOptimizedGraphBuilder::LoadPolymorphicAsMonomorphic(
On 2013/06/13 15:26:36, Hannes Payer wrote:
> Would it make more sense to name this method TryLoadPolymorphicAsMonomorphic?

Done.

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode6365
src/hydrogen.cc:6365: void
HOptimizedGraphBuilder::HandlePolymorphicLoadNamedField(Property* expr,
On 2013/06/13 15:26:36, Hannes Payer wrote:
> Can you move "Property* expr" to the next line?

Done.

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode6369
src/hydrogen.cc:6369: 
On 2013/06/13 15:26:36, Hannes Payer wrote:
> remove this newline

Done.

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode6677
src/hydrogen.cc:6677: if (monomorphic) {
It kinda minimizes the number of branches. We should probably refactor it... but
not now :)

On 2013/06/13 15:26:36, Hannes Payer wrote:
> can you restructure the code 
>   monomorphic = false;
> } 
> if (monomorphic) {
> ...

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode6955
src/hydrogen.cc:6955: // OS::DebugBreak();
On 2013/06/13 15:26:36, Hannes Payer wrote:
> I guess you do not need that anymore.

Done.

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode9261
src/hydrogen.cc:9261: if (monomorphic) {
Same thing here

On 2013/06/13 15:26:36, Hannes Payer wrote:
> can you restructure the code 
>   monomorphic = false;
> } 
> if (monomorphic) {
> ...

https://codereview.chromium.org/16948005/diff/6001/src/hydrogen.cc#newcode9272
src/hydrogen.cc:9272: if (load == NULL) load = BuildLoadNamedGeneric(object,
name, prop);
True, but lets do that in a different CL.

On 2013/06/13 15:26:36, Hannes Payer wrote:
> This sequence of code is exactly the same as in line 6666. I think it makes
> sense to move it into a separate function. WDYT?

[Hannes Payer (out of office), 2013-06-13 16:35:01]

LGTM

[Toon Verwaest, 2013-06-13 16:36:21]

Committed patchset #5 manually as r15126 (presubmit successful).