[DAGCombiner] Fix for PR37667

[samparker]

Summary

While trying to propagate AND masks back to loads, we currently allow one non-load node to be included as a leaf in chain. This fix now limits that node to produce only a single data value.

[spatel]

I'm not familiar with this transform, but the proposal makes sense. But I think it would be easier to follow the logic if it was written as:

// Also ensure that the node to be masked only produces one data result.
NodeToMask = Op.getNode();
unsigned DataValues = 0;
for (unsigned i = 0; i < NodeToMask->getNumValues(); ++i) {
  MVT VT = SDValue(NodeToMask, i).getSimpleValueType();
  if (VT != MVT::Glue && VT != MVT::Other)
    ++DataValues;
  if (DataValues > 1) {
    NodeToMask = nullptr;
    return false;
  }
}
assert(DataValues == 1 && "Node to be masked has no data result?");

Also, I added tests for both bugs (just in case there was logic difference there) with rL334199. Please use utils/update_llc_test_checks.py to update the assertions and rebase here.

[niravd]

This also makes sense to me. Just to build on Sanjays comment, I think recasting DataValues as boolean is nicer.

NodeToMask = Op.getNode();
bool HasValue = false;
for (unsigned i = 0; i < NodeToMask->getNumValues(); ++i) {

MVT VT = SDValue(NodeToMask, i).getSimpleValueType();
if (VT != MVT::Glue && VT != MVT::Other){
  if(HasValue) {
    NodeToMask = nullptr;
    return false;
  }
  HasValue = true;

}

In D47878#1125018, @spatel wrote:

I'm not familiar with this transform, but the proposal makes sense. But I think it would be easier to follow the logic if it was written as:

// Also ensure that the node to be masked only produces one data result.
NodeToMask = Op.getNode();
unsigned DataValues = 0;
for (unsigned i = 0; i < NodeToMask->getNumValues(); ++i) {
  MVT VT = SDValue(NodeToMask, i).getSimpleValueType();
  if (VT != MVT::Glue && VT != MVT::Other)
    ++DataValues;
  if (DataValues > 1) {
    NodeToMask = nullptr;
    return false;
  }
}
assert(DataValues == 1 && "Node to be masked has no data result?");

Also, I added tests for both bugs (just in case there was logic difference there) with rL334199. Please use utils/update_llc_test_checks.py to update the assertions and rebase here.

In D47878#1125018, @spatel wrote:

I'm not familiar with this transform, but the proposal makes sense. But I think it would be easier to follow the logic if it was written as:

// Also ensure that the node to be masked only produces one data result.
NodeToMask = Op.getNode();
unsigned DataValues = 0;
for (unsigned i = 0; i < NodeToMask->getNumValues(); ++i) {
  MVT VT = SDValue(NodeToMask, i).getSimpleValueType();
  if (VT != MVT::Glue && VT != MVT::Other)
    ++DataValues;
  if (DataValues > 1) {
    NodeToMask = nullptr;
    return false;
  }
}
assert(DataValues == 1 && "Node to be masked has no data result?");

Also, I added tests for both bugs (just in case there was logic difference there) with rL334199. Please use utils/update_llc_test_checks.py to update the assertions and rebase here.

[samparker]

Thanks for the feedback. I've removed my new test and updated the existed one. Also modified the code style.

[niravd]

LGTM.

[spatel]

test/CodeGen/X86/backpropmask.ll

4 ↗

(On Diff #150342)

Please remove this FIXME when committing.

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

4005 ↗

(On Diff #150342)

Minor style issue:

for (unsigned i = 0, e = NodeToMask->getNumValues(); i < e; ++i) {

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

case ISD::AND:
                              NodeToMask))
         return false;
       continue;
     }
 
     // Allow one node which will masked along with any loads found.
     if (NodeToMask)
       return false;
+ 
+    // Also ensure that the node to be masked only produces one data result. 
     NodeToMask = Op.getNode();
+    if (NodeToMask->getNumValues() > 1) {
+      bool HasValue = false;
+      for (unsigned i = 0, e = NodeToMask->getNumValues(); i < e; ++i) {
+        MVT VT = SDValue(NodeToMask, i).getSimpleValueType();
+        if (VT != MVT::Glue && VT != MVT::Other) {
+          if (HasValue) {
+            NodeToMask = nullptr;
+            return false;
+          }
+          HasValue = true;
+        }
+      }
+      assert(HasValue && "Node to be masked has no data result?");
+    }
   }
   return true;
 }
 
 bool DAGCombiner::BackwardsPropagateMask(SDNode *N, SelectionDAG &DAG) {
   auto *Mask = dyn_cast<ConstantSDNode>(N->getOperand(1));
   if (!Mask)
     return false;

llvm/trunk/test/CodeGen/X86/backpropmask.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=x86_64-unknown-unknown < %s | FileCheck %s
 
-; FIXME:
-; Verify that backwards propagation of a mask does not affect
-; nodes with multiple result values. In both tests, the stored
-; 32-bit value should be masked to an 8-bit number (and 255).
-
 @b = local_unnamed_addr global i32 918, align 4
 @d = local_unnamed_addr global i32 8089, align 4
 @c = common local_unnamed_addr global i32 0, align 4
 @a = common local_unnamed_addr global i32 0, align 4
 
 define void @PR37667() {
 ; CHECK-LABEL: PR37667:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    movzbl {{.*}}(%rip), %ecx
 ; CHECK-NEXT:    movl {{.*}}(%rip), %eax
 ; CHECK-NEXT:    xorl %edx, %edx
 ; CHECK-NEXT:    divl {{.*}}(%rip)
-; CHECK-NEXT:    orl %ecx, %edx
-; CHECK-NEXT:    movl %edx, {{.*}}(%rip)
+; CHECK-NEXT:    orl {{.*}}(%rip), %edx
+; CHECK-NEXT:    movzbl %dl, %eax
+; CHECK-NEXT:    movl %eax, {{.*}}(%rip)
 ; CHECK-NEXT:    retq
   %t0 = load i32, i32* @c, align 4
   %t1 = load i32, i32* @b, align 4
   %t2 = load i32, i32* @d, align 4
   %rem = urem i32 %t1, %t2
   %or = or i32 %rem, %t0
   %conv1 = and i32 %or, 255
   store i32 %conv1, i32* @a, align 4
   ret void
 }
 
 define void @PR37060() {
 ; CHECK-LABEL: PR37060:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    movl $-1, %eax
 ; CHECK-NEXT:    cltd
 ; CHECK-NEXT:    idivl {{.*}}(%rip)
-; CHECK-NEXT:    movzbl {{.*}}(%rip), %eax
-; CHECK-NEXT:    xorl %edx, %eax
+; CHECK-NEXT:    xorl {{.*}}(%rip), %edx
+; CHECK-NEXT:    movzbl %dl, %eax
 ; CHECK-NEXT:    movl %eax, {{.*}}(%rip)
 ; CHECK-NEXT:    retq
   %t0 = load i32, i32* @c, align 4
   %rem = srem i32 -1, %t0
   %t2 = load i32, i32* @b, align 4
   %xor = xor i32 %t2, %rem
   %conv3 = and i32 %xor, 255
   store i32 %conv3, i32* @a, align 4
   ret void
 }