Collect type information based on stack allocated values for OSR.

Collect type information based on stack allocated values for OSR.

R=jkummerow@chromium.org, rossberg@chromium.org

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

[Jakob Kummerow, 2013-12-12 13:46:46]

LGTM with nit.

https://codereview.chromium.org/100093004/diff/10001/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/100093004/diff/10001/src/typing.cc#newcode134
src/typing.cc:134: ASSERT(0 <= index && index < list->length());
Unnecessary -- List::at() contains the same check.

[rossberg, 2013-12-16 09:57:25]

https://codereview.chromium.org/100093004/diff/50001/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/100093004/diff/50001/src/typing.cc#newcode50
src/typing.cc:50: if (info_->is_osr()) ObserveTypesOnStack();
This should go into the Run method. It is not constructing the visitor, it is
part of collecting the type information.

More importantly, as discussed offline, I think the approach in this CL is not
right for two reasons:

(1) It is not sound. It assumes that the stack types can never change.

(2) It duplicates the store typing.

These two problems are related. To fix them, instead of having separate lists,
you should refine the existing store with the type information from the stack
values -- at the point where the code would continue after OSR (i.e., before
loop bodies with the respective OSR AST id?). The refinement should be
expressible by an Alt effect, I think.

https://codereview.chromium.org/100093004/diff/50001/src/typing.cc#newcode78
src/typing.cc:78: if (value->IsUndefined() || value->IsTheHole()) {
Why is the type None for undefined?

[Yang, 2013-12-16 11:44:19]

On 2013/12/16 09:57:25, rossberg wrote:
> https://codereview.chromium.org/100093004/diff/50001/src/typing.cc
> File src/typing.cc (right):
> 
> https://codereview.chromium.org/100093004/diff/50001/src/typing.cc#newcode50
> src/typing.cc:50: if (info_->is_osr()) ObserveTypesOnStack();
> This should go into the Run method. It is not constructing the visitor, it is
> part of collecting the type information.
> 
> More importantly, as discussed offline, I think the approach in this CL is not
> right for two reasons:
> 
> (1) It is not sound. It assumes that the stack types can never change.
> 
> (2) It duplicates the store typing.
> 
> These two problems are related. To fix them, instead of having separate lists,
> you should refine the existing store with the type information from the stack
> values -- at the point where the code would continue after OSR (i.e., before
> loop bodies with the respective OSR AST id?). The refinement should be
> expressible by an Alt effect, I think.
> 
> https://codereview.chromium.org/100093004/diff/50001/src/typing.cc#newcode78
> src/typing.cc:78: if (value->IsUndefined() || value->IsTheHole()) {
> Why is the type None for undefined?

Addressed comments. Please take another look.

[rossberg, 2013-12-16 11:49:44]

LGTM with nit

https://codereview.chromium.org/100093004/diff/70001/src/typing.h
File src/typing.h (right):

https://codereview.chromium.org/100093004/diff/70001/src/typing.h#newcode89
src/typing.h:89: inline int stack_local_index(int index) { return index; }
Nit: the 'inline' is redundant here.

Is there much of a reason to have these as separate helpers anyway?

[Yang, 2013-12-16 11:58:35]

https://codereview.chromium.org/100093004/diff/70001/src/typing.h
File src/typing.h (right):

https://codereview.chromium.org/100093004/diff/70001/src/typing.h#newcode89
src/typing.h:89: inline int stack_local_index(int index) { return index; }
On 2013/12/16 11:49:44, rossberg wrote:
> Nit: the 'inline' is redundant here.
> 
> Is there much of a reason to have these as separate helpers anyway?

Removed 'inline'.

I use them in case where I don't have the pointer to the Variable, but know the
index and variable type.

[Yang, 2013-12-18 10:39:05]

Committed patchset #6 manually as r18343 (presubmit successful).