Fix polymorphic to monomorphic load to take representation into account.

Fix polymorphic to monomorphic load to take representation into account.

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

[Toon Verwaest, 2013-05-06 11:07:09]

PTAL

[Toon Verwaest, 2013-05-06 17:18:14]

giving to mvstanton

[mvstanton, 2013-05-07 10:17:16]

Nice. Just some nits except can you address the comment in
HandlePolymorphicLoadNamedField()?

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

https://codereview.chromium.org/14966005/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:431: HInstruction* load =
AddInstruction(DoBuildLoadNamedField(
Since you always add the instruction, maybe the builder should just add it?
(Seems like some builders do the add, others don't.)

https://codereview.chromium.org/14966005/diff/5001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14966005/diff/5001/src/hydrogen.cc#newcode7142
src/hydrogen.cc:7142: if (types->length() > 0 && types->length() <=
kMaxLoadPolymorphism) {
I like this if check against kMaxLoadPolymorphism, it is clearer than before.

https://codereview.chromium.org/14966005/diff/5001/src/hydrogen.cc#newcode7185
src/hydrogen.cc:7185: instr->set_position(expr->position());
the instr->set_position() line and ast_context() line are unfortunate in that
they don't seem to have anything to do with the rest of the function. That is
okay, except that they are duplicated. Is there a way to just run those lines
once for the instruction created, whether it is an HLoadNamedFieldPolymorphic()
or a DoBuildLoadNamedField()?

Maybe you have instr = NULL; before the loop case, and then check that value
below the loop. That way you only have to write the lines I'm complaining about
once at the end of the function. You also still got to eliminate carrying all
the previous_field_offset crap out of your loop, since you build the instruction
in the loop.

https://codereview.chromium.org/14966005/diff/5001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/14966005/diff/5001/src/hydrogen.h#newcode985
src/hydrogen.h:985: HLoadNamedField* DoBuildLoadNamedField(HValue* object,
Maybe "Do" isn't necessary, to conform with the other Build... methods.

[Toon Verwaest, 2013-05-07 10:30:24]

addressed comments, ptal again

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

https://codereview.chromium.org/14966005/diff/5001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:431: HInstruction* load =
AddInstruction(DoBuildLoadNamedField(
It seems more in sync with the other builds to not add it; so I'll leave it like
it was.

On 2013/05/07 10:17:17, mvstanton wrote:
> Since you always add the instruction, maybe the builder should just add it?
> (Seems like some builders do the add, others don't.)

https://codereview.chromium.org/14966005/diff/5001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14966005/diff/5001/src/hydrogen.cc#newcode7142
src/hydrogen.cc:7142: if (types->length() > 0 && types->length() <=
kMaxLoadPolymorphism) {
On 2013/05/07 10:17:17, mvstanton wrote:
> I like this if check against kMaxLoadPolymorphism, it is clearer than before.

Done.

https://codereview.chromium.org/14966005/diff/5001/src/hydrogen.cc#newcode7185
src/hydrogen.cc:7185: instr->set_position(expr->position());
On 2013/05/07 10:17:17, mvstanton wrote:
> the instr->set_position() line and ast_context() line are unfortunate in that
> they don't seem to have anything to do with the rest of the function. That is
> okay, except that they are duplicated. Is there a way to just run those lines
> once for the instruction created, whether it is an
HLoadNamedFieldPolymorphic()
> or a DoBuildLoadNamedField()?
> 
> Maybe you have instr = NULL; before the loop case, and then check that value
> below the loop. That way you only have to write the lines I'm complaining
about
> once at the end of the function. You also still got to eliminate carrying all
> the previous_field_offset crap out of your loop, since you build the
instruction
> in the loop.

Done.

https://codereview.chromium.org/14966005/diff/5001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/14966005/diff/5001/src/hydrogen.h#newcode985
src/hydrogen.h:985: HLoadNamedField* DoBuildLoadNamedField(HValue* object,
Unfortunately it's necessary because you can't overload methods between 2
different classes.

On 2013/05/07 10:17:17, mvstanton wrote:
> Maybe "Do" isn't necessary, to conform with the other Build... methods.

[mvstanton, 2013-05-07 10:31:05]

lgtm

[Toon Verwaest, 2013-05-07 10:32:30]

Committed patchset #2 manually as r14565 (presubmit successful).