[NewGVN] Relax conditions when checking safety of memory accesses

[ManuelJBrito]

An operand is unsafe for phi-of-ops when it depends on a memory phi in the same block as the PhiBlock.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: None (ManuelJBrito)

Changes

An operand is unsafe for phi-of-ops when it depends on a memory phi in the same block as the PhiBlock.

---

Full diff: https://github.com/llvm/llvm-project/pull/98609.diff

3 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/NewGVN.cpp (+40-13)
• (modified) llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll (+9-11)
• (modified) llvm/test/Transforms/NewGVN/storeoverstore.ll (+6-6)

diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 4cba196ed688a..3a24a476abfb9 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -2597,34 +2597,61 @@ 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 (!(isa<Instruction>(I) || isa<MemoryAccess>(I)))
       continue;
 
     auto OISIt = OpSafeForPHIOfOps.find(I);
     if (OISIt != OpSafeForPHIOfOps.end())
       return OISIt->second;
 
+    // getBlockForValue only works for instructions and memoryPhis.
+    auto *IBlock = isa<MemoryUseOrDef>(I) ? cast<MemoryUseOrDef>(I)->getBlock()
+                                          : 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, 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, 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->mayReadFromMemory())
+      MA = getMemoryAccess(OrigI);
+    else if (isa<MemoryAccess>(I))
+      MA = cast<MemoryAccess>(I);
+    if (MA) {
+      for (auto *Op : MA->operand_values()) {
+        // Null => LiveOnEntryDef
+        if (!Op)
+          continue;
+        // Stop now if we find an unsafe operand.
+        auto OISIt = OpSafeForPHIOfOps.find(OrigI);
+        if (OISIt != OpSafeForPHIOfOps.end()) {
+          if (!OISIt->second) {
+            OpSafeForPHIOfOps.insert({I, false});
+            return false;
+          }
+          continue;
+        }
+        if (!Visited.insert(Op).second)
+          continue;
+        Worklist.push_back(Op);
+      }
+    }
+    // If its a MemoryAccess there is nothing more to do.
+    if (isa<MemoryAccess>(I))
+      continue;
 
     // Check the operands of the current instruction.
     for (auto *Op : OrigI->operand_values()) {
@@ -2641,7 +2668,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, true});
diff --git a/llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll b/llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll
index 12c3389083cb2..e7e03260b3419 100644
--- a/llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll
+++ b/llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll
@@ -83,12 +83,12 @@ define void @no-alias-store-in-loop(ptr noalias %p, ptr noalias %q) {
 ; 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
@@ -156,11 +156,11 @@ define void @nowrite-function-in-loop(ptr %p) {
 ; 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 +199,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
diff --git a/llvm/test/Transforms/NewGVN/storeoverstore.ll b/llvm/test/Transforms/NewGVN/storeoverstore.ll
index 0e31739b04e3e..2d74fe68a82d4 100644
--- a/llvm/test/Transforms/NewGVN/storeoverstore.ll
+++ b/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

[ManuelJBrito]

ping

[alinas]

I am not clear if you need this generalization or if you can keep the invariant that the worklist only has instructions and MemoryPhis. When checking operands, switch to adding the instruction corresponding to an access that's not a MemoryPhi. For all instructions, check operands for the corresponding MA and for the instruction.

[ManuelJBrito]

I tried something similar but ended up going with this version because I thought the code was easier to understand. I'll update the patch so you can decide which version you prefer.

[ManuelJBrito]

I have updated the PR so that the only MemoryAccesses in the worklist are MemoryPhis.