[turbofan] Emit memory operands for cmp and test on ia32 and x64 when it makes sense.

Emit memory operands for cmp and test on ia32 and x64 when it makes sense.

The InstructionSelector now associates an effect level to every node in a block.

The effect level of a node is the number of non-eliminatable nodes encountered from the beginning of the block to the node itself.

With this change, on ia32 and x64, a load from memory into a register can be replaced by a memory operand if all of the following conditions hold:

1. The only use of the load is in a 32 or 64 bit word comparison.
2. The user node and the load node belong to the same block.
3. The values of the operands have the same size (i.e., no need to zero-extend or sign-extend the result of the load).

BUG=

Committed: https://crrev.com/0e43ff5632d38cfb8b0ea0bc6955d6f252cb9ad8
Cr-Commit-Position: refs/heads/master@{#34187}

[epertoso, 2016-02-17 13:05:20]

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

[Benedikt Meurer, 2016-02-18 05:02:53]

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/code-gene...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/code-gene...
src/compiler/ia32/code-generator-ia32.cc:533: Operand memop =
i.MemoryOperand(&index);
Nit: Please use "operand" instead of "memop" to be consistent with the rest of
the file.

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/code-gene...
src/compiler/ia32/code-generator-ia32.cc:550: Operand memop =
i.MemoryOperand(&index);
Nit: Please use "operand" instead of "memop" to be consistent with the rest of
the file.

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/instructi...
File src/compiler/ia32/instruction-selector-ia32.cc (right):

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/instructi...
src/compiler/ia32/instruction-selector-ia32.cc:1011: bool
CanUseMemOp(InstructionCode opcode, Node* node, Node* input) {
Nit: Please use complete workds as in the rest of the file, i.e.
CanUseMemoryOperand

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/instructi...
src/compiler/ia32/instruction-selector-ia32.cc:1012: if (input->opcode() ==
IrOpcode::kLoad && input->OwnedBy(node)) {
OwnedBy is not enough here. You need to use CanCover.

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/instructi...
src/compiler/ia32/instruction-selector-ia32.cc:1037: if (node &&
CanUseMemOp(opcode, node, left)) {
This is not safe unless you also know that there's is no other instruction in
between that invalidates the load, i.e.

 x = load o, i
 store o, i, v
 cmp x, 1

here you must not combine that load+cmp. As discussed earlier, you'll need to
compute an effect level for this to be safe.

[epertoso, 2016-02-18 10:58:30]

PTAL.

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/code-gene...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/code-gene...
src/compiler/ia32/code-generator-ia32.cc:533: Operand memop =
i.MemoryOperand(&index);
On 2016/02/18 at 05:02:53, Benedikt Meurer wrote:
> Nit: Please use "operand" instead of "memop" to be consistent with the rest of
the file.

Done.

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/code-gene...
src/compiler/ia32/code-generator-ia32.cc:550: Operand memop =
i.MemoryOperand(&index);
On 2016/02/18 at 05:02:53, Benedikt Meurer wrote:
> Nit: Please use "operand" instead of "memop" to be consistent with the rest of
the file.

Done.

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/instructi...
File src/compiler/ia32/instruction-selector-ia32.cc (right):

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/instructi...
src/compiler/ia32/instruction-selector-ia32.cc:1011: bool
CanUseMemOp(InstructionCode opcode, Node* node, Node* input) {
On 2016/02/18 at 05:02:53, Benedikt Meurer wrote:
> Nit: Please use complete workds as in the rest of the file, i.e.
CanUseMemoryOperand

Done.

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/instructi...
src/compiler/ia32/instruction-selector-ia32.cc:1012: if (input->opcode() ==
IrOpcode::kLoad && input->OwnedBy(node)) {
On 2016/02/18 at 05:02:53, Benedikt Meurer wrote:
> OwnedBy is not enough here. You need to use CanCover.

Done. Also added the effect level check.

https://codereview.chromium.org/1706763002/diff/1/src/compiler/ia32/instructi...
src/compiler/ia32/instruction-selector-ia32.cc:1037: if (node &&
CanUseMemOp(opcode, node, left)) {
On 2016/02/18 at 05:02:53, Benedikt Meurer wrote:
> This is not safe unless you also know that there's is no other instruction in
between that invalidates the load, i.e.
> 
>  x = load o, i
>  store o, i, v
>  cmp x, 1
> 
> here you must not combine that load+cmp. As discussed earlier, you'll need to
compute an effect level for this to be safe.

Done in CanUseMemoryOperand.

[Benedikt Meurer, 2016-02-18 11:44:39]

https://codereview.chromium.org/1706763002/diff/20001/src/compiler/ia32/instr...
File src/compiler/ia32/instruction-selector-ia32.cc (right):

https://codereview.chromium.org/1706763002/diff/20001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:993: void
VisitCompare(InstructionSelector* selector, InstructionCode opcode,
Please add a new helper VisitCompare and leave the other one as it is.

https://codereview.chromium.org/1706763002/diff/20001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:1021:
selector->GetEffectLevel(input) != selector->GetEffectLevel(node)) {
Please integrate this with CanCover, i.e. make CanConver check whether the input
operator is not pure, and if so also check the effect level of input and node.

https://codereview.chromium.org/1706763002/diff/20001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:1049: InstructionOperand
inputs[4];
Please inline this into the caller below.

[epertoso, 2016-02-18 13:17:14]

As discussed, I made the changes only in instruction-selector-x64.cc and
InstructionSelector::CanCover.

Seems slightly more readable (still missing the MI cases).

If this is fine, I'll do the same for ia32.

https://codereview.chromium.org/1706763002/diff/20001/src/compiler/ia32/instr...
File src/compiler/ia32/instruction-selector-ia32.cc (right):

https://codereview.chromium.org/1706763002/diff/20001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:993: void
VisitCompare(InstructionSelector* selector, InstructionCode opcode,
On 2016/02/18 at 11:44:39, Benedikt Meurer wrote:
> Please add a new helper VisitCompare and leave the other one as it is.

Done.

https://codereview.chromium.org/1706763002/diff/20001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:1021:
selector->GetEffectLevel(input) != selector->GetEffectLevel(node)) {
On 2016/02/18 at 11:44:39, Benedikt Meurer wrote:
> Please integrate this with CanCover, i.e. make CanConver check whether the
input operator is not pure, and if so also check the effect level of input and
node.

Done.

[epertoso, 2016-02-18 13:31:35]

https://codereview.chromium.org/1706763002/diff/40001/src/compiler/x64/instru...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/1706763002/diff/40001/src/compiler/x64/instru...
src/compiler/x64/instruction-selector-x64.cc:1345: DCHECK(left_operands_count >
0 && left_operands_count <= 3);
Already deleted this.

[Benedikt Meurer, 2016-02-18 13:49:56]

This one looks way better. Let's do this for the start.

[epertoso, 2016-02-18 14:02:21]

Patchset #4 (id:60001) has been deleted

[epertoso, 2016-02-18 14:07:00]

On 2016/02/18 at 13:49:56, bmeurer wrote:
> This one looks way better. Let's do this for the start.

Done.

[epertoso, 2016-02-18 15:24:06]

Patchset #4 (id:80001) has been deleted

[Benedikt Meurer, 2016-02-19 08:34:47]

Please provide a description of what the intent and behavioral changes are, and
please prefix the subject with [turbofan]

https://codereview.chromium.org/1706763002/diff/100001/src/compiler/instructi...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/1706763002/diff/100001/src/compiler/instructi...
src/compiler/instruction-selector.cc:688: for (auto* node : *block) {
Nit: for (Node* const node : *block)

https://codereview.chromium.org/1706763002/diff/100001/src/compiler/instructi...
src/compiler/instruction-selector.cc:689: if (node->opcode() == IrOpcode::kStore
||
Don't test for specific opcodes here, but check if the operator says it's not
eliminatable.

[epertoso, 2016-02-19 09:38:13]

Description was changed from

==========
ia32/x64: emit memory operands for cmp and test when it makes sense.

BUG=
==========

to

==========
Emit memory operands for cmp and test on ia32 and x64 when it makes sense.


The InstructionSelector now associates an effect level to every node in a block.

The effect level of a node is the number of non-eliminatable nodes encountered
from the beginning of the block to the node itself.

With this change, on ia32 and x64, a load from memory into a register can be
replaced by a memory operand if all of the following conditions hold:

1. The only use of the load is in a 32 or 64 bit word comparison.
2. The user node and the load node belong to the same block.
3. The values of the operands have the same size (i.e., no need to zero-extend
or sign-extend the result of the load).


BUG=
==========

[epertoso, 2016-02-19 10:15:31]

Patchset #6 (id:140001) has been deleted

[epertoso, 2016-02-19 10:15:54]

https://codereview.chromium.org/1706763002/diff/100001/src/compiler/instructi...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/1706763002/diff/100001/src/compiler/instructi...
src/compiler/instruction-selector.cc:688: for (auto* node : *block) {
On 2016/02/19 at 08:34:47, Benedikt Meurer wrote:
> Nit: for (Node* const node : *block)

Done.

https://codereview.chromium.org/1706763002/diff/100001/src/compiler/instructi...
src/compiler/instruction-selector.cc:689: if (node->opcode() == IrOpcode::kStore
||
On 2016/02/19 at 08:34:47, Benedikt Meurer wrote:
> Don't test for specific opcodes here, but check if the operator says it's not
eliminatable.

Done.

[Benedikt Meurer, 2016-02-20 17:38:48]

LGTM once comment is addressed.

https://codereview.chromium.org/1706763002/diff/180001/src/compiler/instructi...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/1706763002/diff/180001/src/compiler/instructi...
src/compiler/instruction-selector.cc:689: if
(!node->op()->HasProperty(Operator::kEliminatable)) ++effect_level;
Thinking about this again, it's actually also not correct, since we have calls
that are marked as elimintable, i.e. FastNewObjectStub, yet it would be
incorrect to combine i.e. a load of allocation top across this call. Sorry, but
you'll need to change this again. I think the correct condition for now is:

opcode == kStore || opcode == kCall || opcode == kCheckedStore

[epertoso, 2016-02-22 09:17:07]

https://codereview.chromium.org/1706763002/diff/180001/src/compiler/instructi...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/1706763002/diff/180001/src/compiler/instructi...
src/compiler/instruction-selector.cc:689: if
(!node->op()->HasProperty(Operator::kEliminatable)) ++effect_level;
On 2016/02/20 at 17:38:47, Benedikt Meurer wrote:
> Thinking about this again, it's actually also not correct, since we have calls
that are marked as elimintable, i.e. FastNewObjectStub, yet it would be
incorrect to combine i.e. a load of allocation top across this call. Sorry, but
you'll need to change this again. I think the correct condition for now is:
> 
> opcode == kStore || opcode == kCall || opcode == kCheckedStore

Done.

[epertoso, 2016-02-22 09:17:13]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2016-02-22 09:17:13]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1706763002/#ps200001
(title: "Update.")

[commit-bot: I haz the power, 2016-02-22 09:17:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1706763002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1706763002/200001

[commit-bot: I haz the power, 2016-02-22 09:45:19]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2016-02-22 09:46:31]

Description was changed from

==========
Emit memory operands for cmp and test on ia32 and x64 when it makes sense.


The InstructionSelector now associates an effect level to every node in a block.

The effect level of a node is the number of non-eliminatable nodes encountered
from the beginning of the block to the node itself.

With this change, on ia32 and x64, a load from memory into a register can be
replaced by a memory operand if all of the following conditions hold:

1. The only use of the load is in a 32 or 64 bit word comparison.
2. The user node and the load node belong to the same block.
3. The values of the operands have the same size (i.e., no need to zero-extend
or sign-extend the result of the load).


BUG=
==========

to

==========
Emit memory operands for cmp and test on ia32 and x64 when it makes sense.

The InstructionSelector now associates an effect level to every node in a block.

The effect level of a node is the number of non-eliminatable nodes encountered
from the beginning of the block to the node itself.

With this change, on ia32 and x64, a load from memory into a register can be
replaced by a memory operand if all of the following conditions hold:

1. The only use of the load is in a 32 or 64 bit word comparison.
2. The user node and the load node belong to the same block.
3. The values of the operands have the same size (i.e., no need to zero-extend
or sign-extend the result of the load).

BUG=

Committed: https://crrev.com/0e43ff5632d38cfb8b0ea0bc6955d6f252cb9ad8
Cr-Commit-Position: refs/heads/master@{#34187}
==========

[commit-bot: I haz the power, 2016-02-22 09:46:32]

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