[llvm] c59bc23 - [NewGVN][PHIOFOPS] Relax conditions when checking safety of memory accesses
via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 1 00:52:53 PDT 2023
Author: ManuelJBrito
Date: 2023-08-01T08:51:57+01:00
New Revision: c59bc230e738036050f4c9b1ebde88699eec74bf
URL: https://github.com/llvm/llvm-project/commit/c59bc230e738036050f4c9b1ebde88699eec74bf
DIFF: https://github.com/llvm/llvm-project/commit/c59bc230e738036050f4c9b1ebde88699eec74bf.diff
LOG: [NewGVN][PHIOFOPS] Relax conditions when checking safety of memory accesses
Currently, we consider any instruction that might read from memory to be unsafe for phi-of-ops.
This patch refines that by walking the clobbering memDefs until we either hit a block that
strictly dominates the phi block (safe) or we hit a clobbering memPhi (unsafe).
Differential Revision: https://reviews.llvm.org/D156055
Added:
Modified:
llvm/lib/Transforms/Scalar/NewGVN.cpp
llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll
llvm/test/Transforms/NewGVN/storeoverstore.ll
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 1af40e2c4e6295..07210495888b4d 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -2624,14 +2624,42 @@ bool NewGVN::OpIsSafeForPHIOfOps(Value *V, const BasicBlock *PHIBlock,
}
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;
+
+ if (MemoryAccess *OriginalAccess = getMemoryAccess(OrigI)) {
+ SmallVector<MemoryAccess *, 4> MemAccessWorkList;
+ MemAccessWorkList.push_back(
+ MSSAWalker->getClobberingMemoryAccess(OriginalAccess));
+
+ // We only want memory defs/phis that might alias with the original
+ // access, so if we can, pass the location to the walker.
+ MemoryLocation Loc =
+ MemoryLocation::getOrNone(OrigI).value_or(MemoryLocation());
+ while (!MemAccessWorkList.empty()) {
+ auto *MemAccess = MemAccessWorkList.pop_back_val();
+ if (MSSA->isLiveOnEntryDef(MemAccess))
+ continue;
+
+ // Phi block is dominated - safe.
+ if (DT->properlyDominates(MemAccess->getBlock(), PHIBlock)) {
+ OpSafeForPHIOfOps.insert({I, true});
+ continue;
+ }
+
+ // Clobbering MemoryPhi - unsafe.
+ // Note : Only checking memory phis allows us to skip redundant stores
+ if (isa<MemoryPhi>(MemAccess) &&
+ MemAccess ==
+ MSSAWalker->getClobberingMemoryAccess(MemAccess, Loc)) {
+ OpSafeForPHIOfOps.insert({I, false});
+ return false;
+ }
+
+ // Add potential clobber of the original access.
+ MemAccessWorkList.push_back(MSSAWalker->getClobberingMemoryAccess(
+ cast<MemoryUseOrDef>(MemAccess)));
+ continue;
+ }
+ }
// Check the operands of the current instruction.
for (auto *Op : OrigI->operand_values()) {
diff --git a/llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll b/llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll
index 12c3389083cb24..00110ba9687c2b 100644
--- a/llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll
+++ b/llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll
@@ -77,18 +77,55 @@ 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(
+define void @clobber-in-loop2(ptr %p, ptr %q, i1 %a) {
+; CHECK-LABEL: @clobber-in-loop2(
; CHECK-NEXT: bb56:
; CHECK-NEXT: br label [[BB57:%.*]]
; CHECK: bb57:
; CHECK-NEXT: [[N59:%.*]] = phi i1 [ false, [[BB229:%.*]] ], [ true, [[BB56:%.*]] ]
; CHECK-NEXT: [[IDX:%.*]] = phi i8 [ 0, [[BB56]] ], [ [[INC:%.*]], [[BB229]] ]
+; CHECK-NEXT: call void @f()
+; CHECK-NEXT: br i1 [[A:%.*]], label [[BB229]], label [[BB237:%.*]]
+; CHECK: 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: [[INC]] = add i8 [[IDX]], 1
+; CHECK-NEXT: br i1 [[N63]], label [[BB57]], label [[BB237]]
+; CHECK: bb237:
+; CHECK-NEXT: ret void
+;
+bb56:
+ br label %bb57
+
+bb57:
+ %n59 = phi i1 [ false, %bb229 ], [ true, %bb56 ]
+ %idx = phi i8 [0, %bb56], [%inc, %bb229]
+ call void @f()
+ br i1 %a, label %bb229, label %bb237
+
+bb229:
+ %n60 = load i8, ptr %p
+ %n62 = icmp ne i8 %n60, 2
+ %n63 = or i1 %n59, %n62
+ %inc = add i8 %idx, 1
+ br i1 %n63, label %bb57, label %bb237
+
+bb237:
+ ret void
+}
+
+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: [[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: 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 +187,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 +235,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
@@ -243,3 +277,58 @@ exit: ; preds = %cond.end, %cond.fal
store i32 %sel, ptr %p3, align 4
ret void
}
+
+define void @function-in-branch-in-loop(ptr %p, i8 %a, i8 %x) {
+; CHECK-LABEL: @function-in-branch-in-loop(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[WHILE:%.*]]
+; CHECK: while:
+; CHECK-NEXT: [[PHI:%.*]] = phi i1 [ true, [[ENTRY:%.*]] ], [ false, [[BODY2:%.*]] ]
+; CHECK-NEXT: [[I:%.*]] = phi i8 [ 0, [[ENTRY]] ], [ [[I2:%.*]], [[BODY2]] ]
+; CHECK-NEXT: [[WHILE_COND:%.*]] = icmp ule i8 [[I]], [[X:%.*]]
+; CHECK-NEXT: br i1 [[WHILE_COND]], label [[BODY:%.*]], label [[EXIT:%.*]]
+; CHECK: body:
+; CHECK-NEXT: [[IF_COND:%.*]] = icmp sgt i8 [[A:%.*]], 0
+; CHECK-NEXT: br i1 [[IF_COND]], label [[IF_THEN:%.*]], label [[IF_ELSE:%.*]]
+; CHECK: if.then:
+; CHECK-NEXT: call void @f()
+; CHECK-NEXT: br label [[BODY2]]
+; CHECK: if.else:
+; CHECK-NEXT: br label [[BODY2]]
+; CHECK: body2:
+; CHECK-NEXT: [[I2]] = add i8 [[I]], 1
+; CHECK-NEXT: [[LD:%.*]] = load i1, ptr [[P:%.*]], align 1
+; CHECK-NEXT: [[OR:%.*]] = or i1 [[PHI]], [[LD]]
+; CHECK-NEXT: br i1 [[OR]], label [[WHILE]], label [[EXIT]]
+; CHECK: exit:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %while
+
+while:
+ %phi = phi i1 [ true, %entry ], [ false, %body2 ]
+ %i = phi i8 [ 0, %entry ], [ %i2, %body2 ]
+ %while.cond = icmp ule i8 %i, %x
+ br i1 %while.cond, label %body, label %exit
+
+body:
+ %if.cond = icmp sgt i8 %a, 0
+ br i1 %if.cond, label %if.then, label %if.else
+
+if.then:
+ call void @f()
+ br label %body2
+
+if.else:
+ br label %body2
+
+body2:
+ %i2 = add i8 %i, 1
+ %ld = load i1, ptr %p
+ %or = or i1 %phi, %ld
+ br i1 %or, label %while, label %exit
+
+exit:
+ ret void
+}
diff --git a/llvm/test/Transforms/NewGVN/storeoverstore.ll b/llvm/test/Transforms/NewGVN/storeoverstore.ll
index 0e31739b04e3ed..2d74fe68a82d4b 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