[llvm] a1f5ea5 - Revert "[NewGVN][PHIOFOPS] Relax conditions when checking safety of memory accesses"
via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 28 07:27:12 PDT 2023
Author: ManuelJBrito
Date: 2023-08-28T15:25:16+01:00
New Revision: a1f5ea5b5f1fd9fd7607731af839587c325c42a9
URL: https://github.com/llvm/llvm-project/commit/a1f5ea5b5f1fd9fd7607731af839587c325c42a9
DIFF: https://github.com/llvm/llvm-project/commit/a1f5ea5b5f1fd9fd7607731af839587c325c42a9.diff
LOG: Revert "[NewGVN][PHIOFOPS] Relax conditions when checking safety of memory accesses"
NewGVN isn't enabled by default so some test failures were being missed.
Reverting to do more testing offline.
This reverts commit c59bc230e738036050f4c9b1ebde88699eec74bf.
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 990660fee52611..d4e9e4c6ab74a7 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -2617,41 +2617,14 @@ bool NewGVN::OpIsSafeForPHIOfOps(Value *V, const BasicBlock *PHIBlock,
}
auto *OrigI = cast<Instruction>(I);
-
- 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)));
- }
- }
+ // 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;
// 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 00110ba9687c2b..12c3389083cb24 100644
--- a/llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll
+++ b/llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll
@@ -77,55 +77,18 @@ bb237:
ret void
}
-define void @clobber-in-loop2(ptr %p, ptr %q, i1 %a) {
-; CHECK-LABEL: @clobber-in-loop2(
+; 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: [[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: [[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-NEXT: br i1 [[N63]], label [[BB229]], label [[BB237:%.*]]
; CHECK: bb229:
; CHECK-NEXT: [[INC]] = add i8 [[IDX]], 1
; CHECK-NEXT: store i8 [[INC]], ptr [[Q:%.*]], align 1
@@ -187,16 +150,17 @@ 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: [[PHIOFOPS:%.*]] = phi i1 [ true, [[BB56:%.*]] ], [ [[N62:%.*]], [[BB229:%.*]] ]
-; CHECK-NEXT: [[N59:%.*]] = phi i1 [ false, [[BB229]] ], [ true, [[BB56]] ]
+; 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: br i1 [[PHIOFOPS]], label [[BB229]], label [[BB237:%.*]]
+; 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: bb229:
; CHECK-NEXT: call void @f() #[[ATTR0:[0-9]+]]
; CHECK-NEXT: br label [[BB57]]
@@ -235,7 +199,9 @@ 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: store i32 0, ptr [[P2:%.*]], align 4
+; 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: br i1 [[COND2:%.*]], label [[COND_END:%.*]], label [[EXIT:%.*]]
; CHECK: cond.end:
; CHECK-NEXT: [[LD3:%.*]] = load i16, ptr [[P1:%.*]], align 2
@@ -277,58 +243,3 @@ 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 2d74fe68a82d4b..0e31739b04e3ed 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: [[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-NEXT: [[DOT0:%.*]] = phi i32 [ 10, [[TMP4]] ], [ 5, [[TMP2:%.*]] ]
+; CHECK-NEXT: br i1 [[TMP3]], label [[TMP6:%.*]], label [[TMP8:%.*]]
; CHECK: 6:
-; CHECK-NEXT: br label [[TMP7]]
-; CHECK: 7:
-; CHECK-NEXT: [[DOT1:%.*]] = phi i32 [ [[PHIOFOPS]], [[TMP6]] ], [ [[DOT0]], [[TMP5]] ]
+; 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: ret i32 [[DOT1]]
;
store i32 5, ptr %0, align 4
More information about the llvm-commits
mailing list