[llvm] [NewGVN] Relax conditions when checking safety of memory accesses (PR #98609)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 12 03:17:15 PDT 2024
https://github.com/ManuelJBrito created https://github.com/llvm/llvm-project/pull/98609
An operand is unsafe for phi-of-ops when it depends on a memory phi in the same block as the PhiBlock.
>From 30cd857f14ef6dc7d25343e77b3363ba5ddb7f83 Mon Sep 17 00:00:00 2001
From: ManuelJBrito <manuel.brito at tecnico.ulisboa.pt>
Date: Fri, 12 Jul 2024 11:08:34 +0100
Subject: [PATCH] Relax conditions safety memory access
---
llvm/lib/Transforms/Scalar/NewGVN.cpp | 53 ++++++++++++++-----
.../Transforms/NewGVN/phi-of-ops-loads.ll | 20 ++++---
llvm/test/Transforms/NewGVN/storeoverstore.ll | 12 ++---
3 files changed, 55 insertions(+), 30 deletions(-)
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
More information about the llvm-commits
mailing list