[NewGVN] Relax conditions when checking safety of memory accesses

llvm/lib/Transforms/Scalar/NewGVN.cpp

@@ -2597,34 +2597,63 @@ bool NewGVN::OpIsSafeForPHIOfOps(Value *V, const BasicBlock *PHIBlock,
   Worklist.push_back(V);
   while (!Worklist.empty()) {
     auto *I = Worklist.pop_back_val();
-    if (!isa<Instruction>(I))
+    if (alwaysAvailable(I))
       continue;
+    assert((isa<Instruction>(I) || isa<MemoryPhi>(I)) &&
+           "Should have been an Instruction or MemoryPhi");
 
     auto OISIt = OpSafeForPHIOfOps.find({I, CacheIdx});
     if (OISIt != OpSafeForPHIOfOps.end())
       return OISIt->second;
 
+    auto *IBlock = getBlockForValue(I);
     // Keep walking until we either dominate the phi block, or hit a phi, or run
     // out of things to check.
-    if (DT->properlyDominates(getBlockForValue(I), PHIBlock)) {
+    if (DT->properlyDominates(IBlock, PHIBlock)) {
       OpSafeForPHIOfOps.insert({{I, CacheIdx}, true});
       continue;
     }
     // PHI in the same block.
-    if (isa<PHINode>(I) && getBlockForValue(I) == PHIBlock) {
+    if ((isa<PHINode>(I) || isa<MemoryPhi>(I)) && IBlock == PHIBlock) {
       OpSafeForPHIOfOps.insert({{I, CacheIdx}, false});
       return false;
     }
 
-    auto *OrigI = cast<Instruction>(I);
-    // When we hit an instruction that reads memory (load, call, etc), we must
-    // consider any store that may happen in the loop. For now, we assume the
-    // worst: there is a store in the loop that alias with this read.
-    // The case where the load is outside the loop is already covered by the
-    // dominator check above.
-    // TODO: relax this condition
-    if (OrigI->mayReadFromMemory())
-      return false;
+    auto *OrigI = dyn_cast<Instruction>(I);
+    // When we encounter an instruction that reads memory (load, call, etc.) or
+    // a MemoryAccess, we must ensure that it does not depend on a memory Phi in
+    // PHIBlock. The base cases are already checked above; now we must check its
+    // operands.
+    MemoryAccess *MA = nullptr;
+    if (OrigI && OrigI->mayReadOrWriteMemory())
+      MA = getMemoryAccess(OrigI);
+    else if (isa<MemoryPhi>(I))
+      MA = cast<MemoryPhi>(I);
+    if (MA) {
+      for (auto *Op : MA->operand_values()) {
+        // Null => LiveOnEntryDef
+        if (!Op || MSSA->isLiveOnEntryDef(cast<MemoryAccess>(Op)))
+          continue;
+        // Add the instruction that the memory access represents.
+        if (auto *MUD = dyn_cast<MemoryUseOrDef>(Op))
+          Op = MUD->getMemoryInst();
+        // Stop now if we find an unsafe operand.
+        auto OISIt = OpSafeForPHIOfOps.find({Op, CacheIdx});
+        if (OISIt != OpSafeForPHIOfOps.end()) {
+          if (!OISIt->second) {
+            OpSafeForPHIOfOps.insert({{I, CacheIdx}, false});
+            return false;
+          }
+          continue;
+        }
+        if (!Visited.insert(Op).second)
+          continue;
+        Worklist.push_back(Op);
+      }
+    }
+    // If it's a MemoryPhi there is nothing more to do.
+    if (isa<MemoryPhi>(I))
+      continue;
 
     // Check the operands of the current instruction.
     for (auto *Op : OrigI->operand_values()) {
@@ -2641,7 +2670,7 @@ bool NewGVN::OpIsSafeForPHIOfOps(Value *V, const BasicBlock *PHIBlock,
       }
       if (!Visited.insert(Op).second)
         continue;
-      Worklist.push_back(cast<Instruction>(Op));
+      Worklist.push_back(Op);
     }
   }
   OpSafeForPHIOfOps.insert({{V, CacheIdx}, true});

llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll

@@ -77,18 +77,17 @@ bb237:
   ret void
 }
 
-; TODO: we should support this case
 define void @no-alias-store-in-loop(ptr noalias %p, ptr noalias %q) {
 ; CHECK-LABEL: @no-alias-store-in-loop(
 ; CHECK-NEXT:  bb56:
 ; CHECK-NEXT:    br label [[BB57:%.*]]
 ; CHECK:       bb57:
-; CHECK-NEXT:    [[N59:%.*]] = phi i1 [ false, [[BB229:%.*]] ], [ true, [[BB56:%.*]] ]
+; CHECK-NEXT:    [[PHIOFOPS:%.*]] = phi i1 [ true, [[BB56:%.*]] ], [ [[N62:%.*]], [[BB229:%.*]] ]
+; CHECK-NEXT:    [[N59:%.*]] = phi i1 [ false, [[BB229]] ], [ true, [[BB56]] ]
 ; CHECK-NEXT:    [[IDX:%.*]] = phi i8 [ 0, [[BB56]] ], [ [[INC:%.*]], [[BB229]] ]
 ; CHECK-NEXT:    [[N60:%.*]] = load i8, ptr [[P:%.*]], align 1
-; CHECK-NEXT:    [[N62:%.*]] = icmp ne i8 [[N60]], 2
-; CHECK-NEXT:    [[N63:%.*]] = or i1 [[N59]], [[N62]]
-; CHECK-NEXT:    br i1 [[N63]], label [[BB229]], label [[BB237:%.*]]
+; CHECK-NEXT:    [[N62]] = icmp ne i8 [[N60]], 2
+; CHECK-NEXT:    br i1 [[PHIOFOPS]], label [[BB229]], label [[BB237:%.*]]
 ; CHECK:       bb229:
 ; CHECK-NEXT:    [[INC]] = add i8 [[IDX]], 1
 ; CHECK-NEXT:    store i8 [[INC]], ptr [[Q:%.*]], align 1
@@ -150,17 +149,16 @@ bb237:
   ret void
 }
 
-; TODO: we should support this case
 define void @nowrite-function-in-loop(ptr %p) {
 ; CHECK-LABEL: @nowrite-function-in-loop(
 ; CHECK-NEXT:  bb56:
 ; CHECK-NEXT:    br label [[BB57:%.*]]
 ; CHECK:       bb57:
-; CHECK-NEXT:    [[N59:%.*]] = phi i1 [ false, [[BB229:%.*]] ], [ true, [[BB56:%.*]] ]
+; CHECK-NEXT:    [[PHIOFOPS:%.*]] = phi i1 [ true, [[BB56:%.*]] ], [ [[N62:%.*]], [[BB229:%.*]] ]
+; CHECK-NEXT:    [[N59:%.*]] = phi i1 [ false, [[BB229]] ], [ true, [[BB56]] ]
 ; CHECK-NEXT:    [[N60:%.*]] = load i8, ptr [[P:%.*]], align 1
-; CHECK-NEXT:    [[N62:%.*]] = icmp ne i8 [[N60]], 2
-; CHECK-NEXT:    [[N63:%.*]] = or i1 [[N59]], [[N62]]
-; CHECK-NEXT:    br i1 [[N63]], label [[BB229]], label [[BB237:%.*]]
+; CHECK-NEXT:    [[N62]] = icmp ne i8 [[N60]], 2
+; CHECK-NEXT:    br i1 [[PHIOFOPS]], label [[BB229]], label [[BB237:%.*]]
 ; CHECK:       bb229:
 ; CHECK-NEXT:    call void @f() #[[ATTR0:[0-9]+]]
 ; CHECK-NEXT:    br label [[BB57]]
@@ -199,9 +197,7 @@ define void @issfeoperand(ptr nocapture readonly %array, i1 %cond1, i1 %cond2, p
 ; CHECK-NEXT:    [[PHI1:%.*]] = phi i8 [ [[LD1]], [[COND_TRUE]] ], [ 0, [[FOR_BODY:%.*]] ]
 ; CHECK-NEXT:    [[ARRAYIDX42:%.*]] = getelementptr inbounds [3 x [2 x [1 x i8]]], ptr [[ARRAY]], i64 109, i64 0, i64 0, i64 undef
 ; CHECK-NEXT:    [[LD2:%.*]] = load i8, ptr [[ARRAYIDX42]], align 1
-; CHECK-NEXT:    [[CMP1:%.*]] = icmp ult i8 [[LD2]], [[PHI1]]
-; CHECK-NEXT:    [[ZEXT:%.*]] = zext i1 [[CMP1]] to i32
-; CHECK-NEXT:    store i32 [[ZEXT]], ptr [[P2:%.*]], align 4
+; CHECK-NEXT:    store i32 0, ptr [[P2:%.*]], align 4
 ; CHECK-NEXT:    br i1 [[COND2:%.*]], label [[COND_END:%.*]], label [[EXIT:%.*]]
 ; CHECK:       cond.end:
 ; CHECK-NEXT:    [[LD3:%.*]] = load i16, ptr [[P1:%.*]], align 2

llvm/test/Transforms/NewGVN/storeoverstore.ll

@@ -15,13 +15,13 @@ define i32 @foo(ptr, i32)  {
 ; CHECK:       4:
 ; CHECK-NEXT:    br label [[TMP5]]
 ; CHECK:       5:
-; CHECK-NEXT:    [[DOT0:%.*]] = phi i32 [ 10, [[TMP4]] ], [ 5, [[TMP2:%.*]] ]
-; CHECK-NEXT:    br i1 [[TMP3]], label [[TMP6:%.*]], label [[TMP8:%.*]]
+; CHECK-NEXT:    [[PHIOFOPS:%.*]] = phi i32 [ 10, [[TMP2:%.*]] ], [ 15, [[TMP4]] ]
+; CHECK-NEXT:    [[DOT0:%.*]] = phi i32 [ 10, [[TMP4]] ], [ 5, [[TMP2]] ]
+; CHECK-NEXT:    br i1 [[TMP3]], label [[TMP6:%.*]], label [[TMP7:%.*]]
 ; CHECK:       6:
-; CHECK-NEXT:    [[TMP7:%.*]] = add nsw i32 [[DOT0]], 5
-; CHECK-NEXT:    br label [[TMP8]]
-; CHECK:       8:
-; CHECK-NEXT:    [[DOT1:%.*]] = phi i32 [ [[TMP7]], [[TMP6]] ], [ [[DOT0]], [[TMP5]] ]
+; CHECK-NEXT:    br label [[TMP7]]
+; CHECK:       7:
+; CHECK-NEXT:    [[DOT1:%.*]] = phi i32 [ [[PHIOFOPS]], [[TMP6]] ], [ [[DOT0]], [[TMP5]] ]
 ; CHECK-NEXT:    ret i32 [[DOT1]]
 ;
   store i32 5, ptr %0, align 4