Support load function prototype in hydrogen/lithium.

Support load function prototype in hydrogen/lithium.

[fschneider, 2010-12-21 17:57:52]

LGTM.

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:2640: virtual bool DataEquals(HValue* other) const {
return true; }
I think you can just rely on the implementation the super-class HValue here.

http://codereview.chromium.org/6009005/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6009005/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:1860: // simply miss the cache instead. This
will allow us to allocate a
This comment is not 100% up-to-date, since we deoptimize here and handle this
case in non-optimized code.

[Vitaly Repeshko, 2010-12-22 15:43:41]

Thanks!


-- Vitaly

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:2640: virtual bool DataEquals(HValue* other) const {
return true; }
On 2010/12/21 17:57:52, fschneider wrote:
> I think you can just rely on the implementation the super-class HValue here.

I just wanted to make it explicit that it's required here. I feel safer this
way.

http://codereview.chromium.org/6009005/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6009005/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:1860: // simply miss the cache instead. This
will allow us to allocate a
On 2010/12/21 17:57:52, fschneider wrote:
> This comment is not 100% up-to-date, since we deoptimize here and handle this
> case in non-optimized code.

Updated.

[Kevin Millikin (Chromium), 2011-01-26 11:54:00]

This change is fishy (sorry I didn't see it before).  I will fix it.

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:135: //       HLoadFunctionPrototype
These are intended to be alphabetized.

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:225: V(LoadFunctionPrototype)                     \
So are these.

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:2628: SetFlagMask(kDependsOnFunctionPrototypes);
1. Don't introduce a new kDependsOn... flag if there is nothing that
specifically kChanges... it (there are only so many bits for these flags).  We
use kDependsOnCalls for this case.

2. SetFlagMask takes a flag mask (something like binary ...00001000, ie, a
shifted flag) not a flag (something like decimal 3).  You want SetFlag here. 
It's error prone and we should change it, but in the meantime we need to be
careful.

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:2640: virtual bool DataEquals(HValue* other) const {
return true; }
1. It's not required unless the instruction has kUseGVN.  I think you intend
this one to be GVN-able, so you should set that flag.

2. I think it's a bit error prone to copy the default implementation instead of
inheriting the default implementation when what you want is the default
implementation.  If we change the default for some reason, this class would not
see it.

[Vitaly Repeshko, 2011-01-26 14:06:23]

Thanks for having a look.


-- Vitaly

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:135: //       HLoadFunctionPrototype
On 2011/01/26 11:54:00, Kevin Millikin wrote:
> These are intended to be alphabetized.

Actually do we need this comment at all? It's a PITA to update it and nothing
enforces that what it says is correct.

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:2628: SetFlagMask(kDependsOnFunctionPrototypes);
On 2011/01/26 11:54:00, Kevin Millikin wrote:
> 1. Don't introduce a new kDependsOn... flag if there is nothing that
> specifically kChanges... it (there are only so many bits for these flags).  We
> use kDependsOnCalls for this case.

There's DependsOnMaps and no ChangesMaps. We actually discussed this with
Florian and decided it's nice to have a more precise dependency in general.

> 2. SetFlagMask takes a flag mask (something like binary ...00001000, ie, a
> shifted flag) not a flag (something like decimal 3).  You want SetFlag here. 
> It's error prone and we should change it, but in the meantime we need to be
> careful.

Oops, I totally messed the flags here...

http://codereview.chromium.org/6009005/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:2640: virtual bool DataEquals(HValue* other) const {
return true; }
On 2011/01/26 11:54:00, Kevin Millikin wrote:
> 1. It's not required unless the instruction has kUseGVN.  I think you intend
> this one to be GVN-able, so you should set that flag.

Yeah, see above.

> 2. I think it's a bit error prone to copy the default implementation instead
of
> inheriting the default implementation when what you want is the default
> implementation.  If we change the default for some reason, this class would
not
> see it.

As you and Florian discussed in another thread having it in the base class is
actually unsafe, having it here makes it less likely that we'll forget to update
it in case the instruction gets more data in the future.