[turbofan] Type feedback for numeric comparisons.

[turbofan] Type feedback for numeric comparisons.

Committed: https://crrev.com/a49c4b0a47b8be622cff33df9767cd4aa162e08b
Cr-Commit-Position: refs/heads/master@{#37024}

[Jarin, 2016-06-06 20:53:49]

jarin@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2016-06-07 04:11:00]

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1315:
javascript()->StrictEqual(CompareOperationHints::Any());
We do have type feedback for switches. Can you either extract it from
clause->CompareId() here or add a TODO that we add it later?

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
File src/compiler/js-operator.cc (left):

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.cc:402: V(HasProperty, Operator::kNoProperties, 2, 1)  
          \
See comment above.

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.cc:404: V(InstanceOf, Operator::kNoProperties, 2, 1)   
          \
See comment above.

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
File src/compiler/js-operator.cc (right):

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.cc:403: op->opcode() == IrOpcode::kJSHasProperty ||
HasProperty and InstanceOf don't have CompareOperationHints.

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/simplified...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1042: // Deoptimize on overflow.
This change doesn't seem related to comparisons?

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hint-...
File src/compiler/type-hint-analyzer.cc (right):

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hint-...
src/compiler/type-hint-analyzer.cc:54: bool
TypeHintAnalysis::GetCompareOperationHints(
Please avoid the detour via types here, and do a simple switch on the state
(which is public in case of the CompareICState). Also don't even bother
extracting a map, as long as we don't use it.

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hints.cc
File src/compiler/type-hints.cc (right):

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hints...
src/compiler/type-hints.cc:106: case BinaryOperationHints::kNone:
This change doesn't seem necessary?

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hints...
src/compiler/type-hints.cc:130: return BinaryOperationHints::kAny;
This change doesn't seem necessary?

[Jarin, 2016-06-07 15:29:29]

Thank for early feedback! I tried to address all your excellent comments.
Unfortunately, this CL does not play well with checkpoints, so it is not ready
for landing yet.

The main problem is the JSToNumbers introduced by typed lowering because they do
not necessarily have a Checkpoint after them.

Also, the deoptimization in simplified lowering Int32AddWithOverflow does not
necessarily find a good frame state (e.g., after LoadBuffer lowering), so I'd
better introduce a sentinel deoptimization that gets frame state from
effect-control-linearizer.

I will get back to this CL once I fix those two issues.


On 2016/06/07 04:11:00, Benedikt Meurer wrote:
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/ast-graph-...
> File src/compiler/ast-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/ast-graph-...
> src/compiler/ast-graph-builder.cc:1315:
> javascript()->StrictEqual(CompareOperationHints::Any());
> We do have type feedback for switches. Can you either extract it from
> clause->CompareId() here or add a TODO that we add it later?
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
> File src/compiler/js-operator.cc (left):
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
> src/compiler/js-operator.cc:402: V(HasProperty, Operator::kNoProperties, 2, 1)
 
>           \
> See comment above.
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
> src/compiler/js-operator.cc:404: V(InstanceOf, Operator::kNoProperties, 2, 1) 
 
>           \
> See comment above.
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
> File src/compiler/js-operator.cc (right):
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
> src/compiler/js-operator.cc:403: op->opcode() == IrOpcode::kJSHasProperty ||
> HasProperty and InstanceOf don't have CompareOperationHints.
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/simplified...
> File src/compiler/simplified-lowering.cc (right):
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/simplified...
> src/compiler/simplified-lowering.cc:1042: // Deoptimize on overflow.
> This change doesn't seem related to comparisons?
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hint-...
> File src/compiler/type-hint-analyzer.cc (right):
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hint-...
> src/compiler/type-hint-analyzer.cc:54: bool
> TypeHintAnalysis::GetCompareOperationHints(
> Please avoid the detour via types here, and do a simple switch on the state
> (which is public in case of the CompareICState). Also don't even bother
> extracting a map, as long as we don't use it.
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hints.cc
> File src/compiler/type-hints.cc (right):
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hints...
> src/compiler/type-hints.cc:106: case BinaryOperationHints::kNone:
> This change doesn't seem necessary?
> 
>
https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hints...
> src/compiler/type-hints.cc:130: return BinaryOperationHints::kAny;
> This change doesn't seem necessary?

[Jarin, 2016-06-09 13:37:32]

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1315:
javascript()->StrictEqual(CompareOperationHints::Any());
On 2016/06/07 04:11:00, Benedikt Meurer wrote:
> We do have type feedback for switches. Can you either extract it from
> clause->CompareId() here or add a TODO that we add it later?

Done.

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
File src/compiler/js-operator.cc (left):

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.cc:402: V(HasProperty, Operator::kNoProperties, 2, 1)  
          \
On 2016/06/07 04:11:00, Benedikt Meurer wrote:
> See comment above.

Done.

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.cc:404: V(InstanceOf, Operator::kNoProperties, 2, 1)   
          \
On 2016/06/07 04:11:00, Benedikt Meurer wrote:
> See comment above.

Done.

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
File src/compiler/js-operator.cc (right):

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.cc:403: op->opcode() == IrOpcode::kJSHasProperty ||
On 2016/06/07 04:11:00, Benedikt Meurer wrote:
> HasProperty and InstanceOf don't have CompareOperationHints.

Done.

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/simplified...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:1042: // Deoptimize on overflow.
On 2016/06/07 04:11:00, Benedikt Meurer wrote:
> This change doesn't seem related to comparisons?

Done.

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hint-...
File src/compiler/type-hint-analyzer.cc (right):

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hint-...
src/compiler/type-hint-analyzer.cc:54: bool
TypeHintAnalysis::GetCompareOperationHints(
On 2016/06/07 04:11:00, Benedikt Meurer wrote:
> Please avoid the detour via types here, and do a simple switch on the state
> (which is public in case of the CompareICState). Also don't even bother
> extracting a map, as long as we don't use it.

Done.

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hints.cc
File src/compiler/type-hints.cc (right):

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hints...
src/compiler/type-hints.cc:106: case BinaryOperationHints::kNone:
On 2016/06/07 04:11:00, Benedikt Meurer wrote:
> This change doesn't seem necessary?

Done.

https://codereview.chromium.org/2035383003/diff/20001/src/compiler/type-hints...
src/compiler/type-hints.cc:130: return BinaryOperationHints::kAny;
On 2016/06/07 04:11:00, Benedikt Meurer wrote:
> This change doesn't seem necessary?

Done.

[Benedikt Meurer, 2016-06-16 05:52:17]

lgtm

[Jarin, 2016-06-16 06:33:04]

The CQ bit was checked by jarin@chromium.org

[commit-bot: I haz the power, 2016-06-16 06:33:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2035383003/140001

[commit-bot: I haz the power, 2016-06-16 06:34:55]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-16 06:37:39]

Description was changed from

==========
[turbofan] Type feedback for numeric comparisons.
==========

to

==========
[turbofan] Type feedback for numeric comparisons.

Committed: https://crrev.com/a49c4b0a47b8be622cff33df9767cd4aa162e08b
Cr-Commit-Position: refs/heads/master@{#37024}
==========

[commit-bot: I haz the power, 2016-06-16 06:37:41]

Patchset 8 (id:??) landed as
https://crrev.com/a49c4b0a47b8be622cff33df9767cd4aa162e08b
Cr-Commit-Position: refs/heads/master@{#37024}