[DAG] Fix Memory ordering check in ReduceLoadOpStore.

[niravd]

Summary

When merging through a TokenFactor we need to check that the load may
be ordered such that no other aliasing memory operations may
happen. It is not sufficient to just check that the load is a member
of the chain token factor; we must check it is not an indirect check.

This fixes PR37826.

[spatel]

Thanks!
Adding potential reviewers (I still don't have a good understanding). Bug link:
https://bugs.llvm.org/show_bug.cgi?id=37826

It would be better to remove the main() code in the test file, auto-generate the full output showing the buggy memop sequence, and commit that against trunk. Then, when you regenerate the results after this patch, we'll see that the load and stores have changed.

[efriedma]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

13064 ↗

(On Diff #155714)

Instead of doing this predecessor check, could we just check that the load's chain has one use? I guess that misses some cases that could be transformed, but I'd like to avoid these expensive walks as much as possible.

[niravd]

Remove the expensive check in favor of more conservative check that LD's chain has one use.

[efriedma]

LGTM

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

if (V->getOpcode() != ISD::AND ||
       !isa<ConstantSDNode>(V->getOperand(1)) ||
       !ISD::isNormalLoad(V->getOperand(0).getNode()))
     return Result;
 
   // Check the chain and pointer.
   LoadSDNode *LD = cast<LoadSDNode>(V->getOperand(0));
   if (LD->getBasePtr() != Ptr) return Result;  // Not from same pointer.
 
-  // The store should be chained directly to the load or be an operand of a
-  // tokenfactor.
-  if (LD == Chain.getNode())
-    ; // ok.
-  else if (Chain->getOpcode() != ISD::TokenFactor)
-    return Result; // Fail.
-  else {
-    bool isOk = false;
-    for (const SDValue &ChainOp : Chain->op_values())
-      if (ChainOp.getNode() == LD) {
-        isOk = true;
-        break;
-      }
-    if (!isOk) return Result;
-  }
-
   // This only handles simple types.
   if (V.getValueType() != MVT::i16 &&
       V.getValueType() != MVT::i32 &&
       V.getValueType() != MVT::i64)
     return Result;
 
   // Check the constant mask.  Invert it so that the bits being masked out are
   // 0 and the bits being kept are 1.  Use getSExtValue so that leading bits
CheckForMaskedLoad(SDValue V, SDValue Ptr, SDValue Chain) {
   case 4: break;
   default: return Result; // All one mask, or 5-byte mask.
   }
 
   // Verify that the first bit starts at a multiple of mask so that the access
   // is aligned the same as the access width.
   if (NotMaskTZ && NotMaskTZ/8 % MaskedBytes) return Result;
 
+  // For narrowing to be valid, it must be the case that the load the
+  // immediately preceeding memory operation before the store.
+  if (LD == Chain.getNode())
+    ; // ok.
+  else if (Chain->getOpcode() == ISD::TokenFactor &&
+           SDValue(LD, 1).hasOneUse()) {
+    // LD has only 1 chain use so they are no indirect dependencies.
+    bool isOk = false;
+    for (const SDValue &ChainOp : Chain->op_values())
+      if (ChainOp.getNode() == LD) {
+        isOk = true;
+        break;
+      }
+    if (!isOk)
+      return Result;
+  } else
+    return Result; // Fail.
+
   Result.first = MaskedBytes;
   Result.second = NotMaskTZ/8;
   return Result;
 }
 
 /// Check to see if IVal is something that provides a value as specified by
 /// MaskInfo. If so, replace the specified store with a narrower store of
 /// truncated IVal.

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

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -o - %s -mtriple=x86_64--unknown-linux-gnu | FileCheck %s
 
 ; When compiled and run this should print zero.
 
 
 @c = common local_unnamed_addr global i32 0, align 4
 @f = common local_unnamed_addr global i32 0, align 4
 @e = common local_unnamed_addr global i32 0, align 4
 @.str.1 = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
 
-; FIXME The generated code here is incorrect. We should only see a
-; single store to f (a bytes store to f+3).
+; We should only see a single store to f (a bytes store to f+3).
 define void @k(i32 %l) {
 ; CHECK-LABEL: k:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    movl {{.*}}(%rip), %eax
-; CHECK-NEXT:    movl %eax, %ecx
-; CHECK-NEXT:    andl $2097151, %ecx # imm = 0x1FFFFF
-; CHECK-NEXT:    xorl {{.*}}(%rip), %ecx
-; CHECK-NEXT:    xorl $2097151, %ecx # imm = 0x1FFFFF
-; CHECK-NEXT:    andl $-16777216, %eax # imm = 0xFF000000
-; CHECK-NEXT:    orl %ecx, %eax
-; CHECK-NEXT:    movl %eax, {{.*}}(%rip)
+; CHECK-NEXT:    orl {{.*}}(%rip), %eax
 ; CHECK-NEXT:    shrl $24, %eax
 ; CHECK-NEXT:    movb %al, f+{{.*}}(%rip)
 ; CHECK-NEXT:    retq
   %load = load i32, i32* @c, align 4
   %load6 = load i32, i32* @f, align 4
   %clear7 = and i32 %load6, 16777215
   store i32 %clear7, i32* @c, align 4
   %neg = and i32 %load6, 2097151