[x64/ia32] Deal with the non-transitivity of InstructionSelector::CanCover()...

[x64/ia32] Deal with the non-transitivity of InstructionSelector::CanCover() when folding loads into branches.

Sequences like:

1: Load[kRepWord32|kTypeInt32](<address>, ...)
2: Word32And(1, <constant>)
3: Word32Equal(2, <another constant>)
4: Store[(kRepWord32 : NoWriteBarrier)](<address>, <value>)
5: Branch[None](3, ...) -> B1, B2

where #1 and #4 refer to the same memory location, are problematic because in VisitBranch we assume that 'InstructionSelector::CanCover()' is transitive.

What happens is that CanCover(5, 3) is true (3 is a pure op), and so are CanCover(3, 2), CanCover(2, 1), but the effect level of 5 and 3 never gets checked because 3 is a pure op. Upon VisitBranch, we ended up materializing:

mov [address], <value>
test [address], <another constant>

With this patch, it becomes:

mov reg, [address]
mov [address], <value>
test reg, <another constant>

BUG=chromium:611976

Review-Url: https://codereview.chromium.org/2008493002
Cr-Commit-Position: refs/heads/master@{#36482}

src/compiler/ia32/instruction-selector-ia32.cc

@@ -27,11 +27,15 @@ class IA32OperandGenerator final : public OperandGenerator {
     return DefineAsRegister(node);
   }
 
-  bool CanBeMemoryOperand(InstructionCode opcode, Node* node, Node* input) {
+  bool CanBeMemoryOperand(InstructionCode opcode, Node* node, Node* input,
+                          int effect_level) {
     if (input->opcode() != IrOpcode::kLoad ||
         !selector()->CanCover(node, input)) {
       return false;
     }
+    if (effect_level != selector()->GetEffectLevel(input)) {
+      return false;
+    }
     MachineRepresentation rep =
         LoadRepresentationOf(input->op()).representation();
     switch (opcode) {
@@ -1235,18 +1239,24 @@ void VisitWordCompare(InstructionSelector* selector, Node* node,
 
   InstructionCode narrowed_opcode = TryNarrowOpcodeSize(opcode, left, right);
 
+  int effect_level = selector->GetEffectLevel(node);
+  if (cont->IsBranch()) {
+    effect_level = selector->GetEffectLevel(
+        cont->true_block()->PredecessorAt(0)->control_input());
+  }
+
   // If one of the two inputs is an immediate, make sure it's on the right, or
   // if one of the two inputs is a memory operand, make sure it's on the left.
   if ((!g.CanBeImmediate(right) && g.CanBeImmediate(left)) ||
-      (g.CanBeMemoryOperand(narrowed_opcode, node, right) &&
-       !g.CanBeMemoryOperand(narrowed_opcode, node, left))) {
+      (g.CanBeMemoryOperand(narrowed_opcode, node, right, effect_level) &&
+       !g.CanBeMemoryOperand(narrowed_opcode, node, left, effect_level))) {
     if (!node->op()->HasProperty(Operator::kCommutative)) cont->Commute();
     std::swap(left, right);
   }
 
   // Match immediates on right side of comparison.
   if (g.CanBeImmediate(right)) {
-    if (g.CanBeMemoryOperand(opcode, node, left)) {
+    if (g.CanBeMemoryOperand(opcode, node, left, effect_level)) {
       // TODO(epertoso): we should use `narrowed_opcode' here once we match
       // immediates too.
       return VisitCompareWithMemoryOperand(selector, opcode, left,
@@ -1257,7 +1267,7 @@ void VisitWordCompare(InstructionSelector* selector, Node* node,
   }
 
   // Match memory operands on left side of comparison.
-  if (g.CanBeMemoryOperand(narrowed_opcode, node, left)) {
+  if (g.CanBeMemoryOperand(narrowed_opcode, node, left, effect_level)) {
     bool needs_byte_register =
         narrowed_opcode == kIA32Test8 || narrowed_opcode == kIA32Cmp8;
     return VisitCompareWithMemoryOperand(

src/compiler/instruction-selector.cc

@@ -713,6 +713,12 @@ void InstructionSelector::VisitBlock(BasicBlock* block) {
     SetEffectLevel(node, effect_level);
   }
 
+  // We visit the control first, then the nodes in the block, so the block's
+  // control input should be on the same effect level as the last node.
+  if (block->control_input() != nullptr) {
+    SetEffectLevel(block->control_input(), effect_level);
+  }
+
   // Generate code for the block control "top down", but schedule the code
   // "bottom up".
   VisitControl(block);

src/compiler/x64/instruction-selector-x64.cc

@@ -37,11 +37,15 @@ class X64OperandGenerator final : public OperandGenerator {
     }
   }
 
-  bool CanBeMemoryOperand(InstructionCode opcode, Node* node, Node* input) {
+  bool CanBeMemoryOperand(InstructionCode opcode, Node* node, Node* input,
+                          int effect_level) {
     if (input->opcode() != IrOpcode::kLoad ||
         !selector()->CanCover(node, input)) {
       return false;
     }
+    if (effect_level != selector()->GetEffectLevel(input)) {
+      return false;
+    }
     MachineRepresentation rep =
         LoadRepresentationOf(input->op()).representation();
     switch (opcode) {
@@ -1548,16 +1552,22 @@ void VisitWordCompare(InstructionSelector* selector, Node* node,
 
   // If one of the two inputs is an immediate, make sure it's on the right, or
   // if one of the two inputs is a memory operand, make sure it's on the left.
+  int effect_level = selector->GetEffectLevel(node);
+  if (cont->IsBranch()) {
+    effect_level = selector->GetEffectLevel(
+        cont->true_block()->PredecessorAt(0)->control_input());
+  }
+
   if ((!g.CanBeImmediate(right) && g.CanBeImmediate(left)) ||
-      (g.CanBeMemoryOperand(opcode, node, right) &&
-       !g.CanBeMemoryOperand(opcode, node, left))) {
+      (g.CanBeMemoryOperand(opcode, node, right, effect_level) &&
+       !g.CanBeMemoryOperand(opcode, node, left, effect_level))) {
     if (!node->op()->HasProperty(Operator::kCommutative)) cont->Commute();
     std::swap(left, right);
   }
 
   // Match immediates on right side of comparison.
   if (g.CanBeImmediate(right)) {
-    if (g.CanBeMemoryOperand(opcode, node, left)) {
+    if (g.CanBeMemoryOperand(opcode, node, left, effect_level)) {
       return VisitCompareWithMemoryOperand(selector, opcode, left,
                                            g.UseImmediate(right), cont);
     }
@@ -1566,7 +1576,7 @@ void VisitWordCompare(InstructionSelector* selector, Node* node,
   }
 
   // Match memory operands on left side of comparison.
-  if (g.CanBeMemoryOperand(opcode, node, left)) {
+  if (g.CanBeMemoryOperand(opcode, node, left, effect_level)) {
     return VisitCompareWithMemoryOperand(selector, opcode, left,
                                          g.UseRegister(right), cont);
   }

test/cctest/compiler/test-branch-combine.cc

@@ -457,6 +457,27 @@ TEST(BranchCombineFloat64Compares) {
   }
 }
 
+TEST(BranchCombineEffectLevel) {
+  // Test that the load doesn't get folded into the branch, as there's a store
+  // between them. See http://crbug.com/611976.
+  int32_t input = 0;
+
+  RawMachineAssemblerTester<int32_t> m;
+  Node* a = m.LoadFromPointer(&input, MachineType::Int32());
+  Node* compare = m.Word32And(a, m.Int32Constant(1));
+  Node* equal = m.Word32Equal(compare, m.Int32Constant(0));
+  m.StoreToPointer(&input, MachineRepresentation::kWord32, m.Int32Constant(1));
+
+  RawMachineLabel blocka, blockb;
+  m.Branch(equal, &blocka, &blockb);
+  m.Bind(&blocka);
+  m.Return(m.Int32Constant(42));
+  m.Bind(&blockb);
+  m.Return(m.Int32Constant(0));
+
+  CHECK_EQ(42, m.Call());
+}
+
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8