[llvm] 43b7dfc - Revert "[SROA] Unfold gep of index phi (#83087)"
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 28 15:56:48 PST 2024
Author: Fangrui Song
Date: 2024-02-28T15:56:43-08:00
New Revision: 43b7dfcc1dbdf904b1c507e03e5c34d527b5a344
URL: https://github.com/llvm/llvm-project/commit/43b7dfcc1dbdf904b1c507e03e5c34d527b5a344
DIFF: https://github.com/llvm/llvm-project/commit/43b7dfcc1dbdf904b1c507e03e5c34d527b5a344.diff
LOG: Revert "[SROA] Unfold gep of index phi (#83087)"
This reverts commit 2eb63982e88b9ed8336158d35884b1a1d04a0f78.
This caused verifier error
```
Instruction does not dominate all uses!
```
for some projects using Halide.
The verifier error happens inside `Halide::Internal::CodeGen_LLVM::optimize_module`
and looks like a genuine SROA issue.
Added:
Modified:
llvm/lib/Transforms/Scalar/SROA.cpp
llvm/test/Transforms/SROA/phi-and-select.ll
llvm/test/Transforms/SROA/phi-gep.ll
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp
index c7b9ce2e93120a..fad70e8bf2861f 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -3956,11 +3956,11 @@ class AggLoadStoreRewriter : public InstVisitor<AggLoadStoreRewriter, bool> {
return false;
}
- // Unfold gep (select cond, ptr1, ptr2), idx
+ // Fold gep (select cond, ptr1, ptr2), idx
// => select cond, gep(ptr1, idx), gep(ptr2, idx)
// and gep ptr, (select cond, idx1, idx2)
// => select cond, gep(ptr, idx1), gep(ptr, idx2)
- bool unfoldGEPSelect(GetElementPtrInst &GEPI) {
+ bool foldGEPSelect(GetElementPtrInst &GEPI) {
// Check whether the GEP has exactly one select operand and all indices
// will become constant after the transform.
SelectInst *Sel = dyn_cast<SelectInst>(GEPI.getPointerOperand());
@@ -4029,100 +4029,67 @@ class AggLoadStoreRewriter : public InstVisitor<AggLoadStoreRewriter, bool> {
return true;
}
- // Unfold gep (phi ptr1, ptr2), idx
- // => phi ((gep ptr1, idx), (gep ptr2, idx))
- // and gep ptr, (phi idx1, idx2)
- // => phi ((gep ptr, idx1), (gep ptr, idx2))
- bool unfoldGEPPhi(GetElementPtrInst &GEPI) {
- // To prevent infinitely expanding recursive phis, bail if the GEP pointer
- // operand (looking through the phi if it is the phi we want to unfold) is
- // an instruction besides an alloca.
- PHINode *Phi = dyn_cast<PHINode>(GEPI.getPointerOperand());
- auto IsInvalidPointerOperand = [](Value *V) {
- return isa<Instruction>(V) && !isa<AllocaInst>(V);
- };
- if (Phi) {
- if (any_of(Phi->operands(), IsInvalidPointerOperand))
- return false;
- } else {
- if (IsInvalidPointerOperand(GEPI.getPointerOperand()))
- return false;
- }
- // Check whether the GEP has exactly one phi operand (including the pointer
- // operand) and all indices will become constant after the transform.
- for (Value *Op : GEPI.indices()) {
- if (auto *SI = dyn_cast<PHINode>(Op)) {
- if (Phi)
- return false;
-
- Phi = SI;
- if (!all_of(Phi->incoming_values(),
- [](Value *V) { return isa<ConstantInt>(V); }))
- return false;
- continue;
- }
-
- if (!isa<ConstantInt>(Op))
- return false;
- }
+ // Fold gep (phi ptr1, ptr2) => phi gep(ptr1), gep(ptr2)
+ bool foldGEPPhi(GetElementPtrInst &GEPI) {
+ if (!GEPI.hasAllConstantIndices())
+ return false;
- if (!Phi)
+ PHINode *PHI = cast<PHINode>(GEPI.getPointerOperand());
+ if (GEPI.getParent() != PHI->getParent() ||
+ llvm::any_of(PHI->incoming_values(), [](Value *In) {
+ Instruction *I = dyn_cast<Instruction>(In);
+ return !I || isa<GetElementPtrInst>(I) || isa<PHINode>(I) ||
+ succ_empty(I->getParent()) ||
+ !I->getParent()->isLegalToHoistInto();
+ }))
return false;
LLVM_DEBUG(dbgs() << " Rewriting gep(phi) -> phi(gep):\n";
- dbgs() << " original: " << *Phi << "\n";
+ dbgs() << " original: " << *PHI << "\n";
dbgs() << " " << GEPI << "\n";);
- auto GetNewOps = [&](Value *PhiOp) {
- SmallVector<Value *> NewOps;
- for (Value *Op : GEPI.operands())
- if (Op == Phi)
- NewOps.push_back(PhiOp);
- else
- NewOps.push_back(Op);
- return NewOps;
- };
-
- IRB.SetInsertPoint(Phi);
- PHINode *NewPhi = IRB.CreatePHI(GEPI.getType(), Phi->getNumIncomingValues(),
- Phi->getName() + ".sroa.phi");
-
+ SmallVector<Value *, 4> Index(GEPI.indices());
bool IsInBounds = GEPI.isInBounds();
- Type *SourceTy = GEPI.getSourceElementType();
- // We only handle arguments, constants, and static allocas here, so we can
- // insert GEPs at the beginning of the function after static allocas.
- IRB.SetInsertPointPastAllocas(GEPI.getFunction());
- for (unsigned I = 0, E = Phi->getNumIncomingValues(); I != E; ++I) {
- Value *Op = Phi->getIncomingValue(I);
- BasicBlock *BB = Phi->getIncomingBlock(I);
- SmallVector<Value *> NewOps = GetNewOps(Op);
-
- Value *NewGEP =
- IRB.CreateGEP(SourceTy, NewOps[0], ArrayRef(NewOps).drop_front(),
- Phi->getName() + ".sroa.gep", IsInBounds);
- NewPhi->addIncoming(NewGEP, BB);
+ IRB.SetInsertPoint(GEPI.getParent(), GEPI.getParent()->getFirstNonPHIIt());
+ PHINode *NewPN = IRB.CreatePHI(GEPI.getType(), PHI->getNumIncomingValues(),
+ PHI->getName() + ".sroa.phi");
+ for (unsigned I = 0, E = PHI->getNumIncomingValues(); I != E; ++I) {
+ BasicBlock *B = PHI->getIncomingBlock(I);
+ Value *NewVal = nullptr;
+ int Idx = NewPN->getBasicBlockIndex(B);
+ if (Idx >= 0) {
+ NewVal = NewPN->getIncomingValue(Idx);
+ } else {
+ Instruction *In = cast<Instruction>(PHI->getIncomingValue(I));
+
+ IRB.SetInsertPoint(In->getParent(), std::next(In->getIterator()));
+ Type *Ty = GEPI.getSourceElementType();
+ NewVal = IRB.CreateGEP(Ty, In, Index, In->getName() + ".sroa.gep",
+ IsInBounds);
+ }
+ NewPN->addIncoming(NewVal, B);
}
Visited.erase(&GEPI);
- GEPI.replaceAllUsesWith(NewPhi);
+ GEPI.replaceAllUsesWith(NewPN);
GEPI.eraseFromParent();
- Visited.insert(NewPhi);
- enqueueUsers(*NewPhi);
+ Visited.insert(NewPN);
+ enqueueUsers(*NewPN);
LLVM_DEBUG(dbgs() << " to: ";
for (Value *In
- : NewPhi->incoming_values()) dbgs()
+ : NewPN->incoming_values()) dbgs()
<< "\n " << *In;
- dbgs() << "\n " << *NewPhi << '\n');
+ dbgs() << "\n " << *NewPN << '\n');
return true;
}
bool visitGetElementPtrInst(GetElementPtrInst &GEPI) {
- if (unfoldGEPSelect(GEPI))
+ if (foldGEPSelect(GEPI))
return true;
- if (unfoldGEPPhi(GEPI))
+ if (isa<PHINode>(GEPI.getPointerOperand()) && foldGEPPhi(GEPI))
return true;
enqueueUsers(GEPI);
diff --git a/llvm/test/Transforms/SROA/phi-and-select.ll b/llvm/test/Transforms/SROA/phi-and-select.ll
index 7c8b27c9de9c0b..54cfb10793a1ac 100644
--- a/llvm/test/Transforms/SROA/phi-and-select.ll
+++ b/llvm/test/Transforms/SROA/phi-and-select.ll
@@ -114,13 +114,13 @@ define i32 @test3(i32 %x) {
; CHECK-LABEL: @test3(
; CHECK-NEXT: entry:
; CHECK-NEXT: switch i32 [[X:%.*]], label [[BB0:%.*]] [
-; CHECK-NEXT: i32 1, label [[BB1:%.*]]
-; CHECK-NEXT: i32 2, label [[BB2:%.*]]
-; CHECK-NEXT: i32 3, label [[BB3:%.*]]
-; CHECK-NEXT: i32 4, label [[BB4:%.*]]
-; CHECK-NEXT: i32 5, label [[BB5:%.*]]
-; CHECK-NEXT: i32 6, label [[BB6:%.*]]
-; CHECK-NEXT: i32 7, label [[BB7:%.*]]
+; CHECK-NEXT: i32 1, label [[BB1:%.*]]
+; CHECK-NEXT: i32 2, label [[BB2:%.*]]
+; CHECK-NEXT: i32 3, label [[BB3:%.*]]
+; CHECK-NEXT: i32 4, label [[BB4:%.*]]
+; CHECK-NEXT: i32 5, label [[BB5:%.*]]
+; CHECK-NEXT: i32 6, label [[BB6:%.*]]
+; CHECK-NEXT: i32 7, label [[BB7:%.*]]
; CHECK-NEXT: ]
; CHECK: bb0:
; CHECK-NEXT: br label [[EXIT:%.*]]
@@ -733,7 +733,6 @@ define void @PR20822(i1 %c1, i1 %c2, ptr %ptr) {
; CHECK-LABEL: @PR20822(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[F_SROA_0:%.*]] = alloca i32, align 4
-; CHECK-NEXT: [[F1_SROA_GEP:%.*]] = getelementptr inbounds [[STRUCT_S:%.*]], ptr [[PTR:%.*]], i32 0, i32 0
; CHECK-NEXT: br i1 [[C1:%.*]], label [[IF_END:%.*]], label [[FOR_COND:%.*]]
; CHECK: for.cond:
; CHECK-NEXT: br label [[IF_END]]
@@ -743,8 +742,9 @@ define void @PR20822(i1 %c1, i1 %c2, ptr %ptr) {
; CHECK: if.then2:
; CHECK-NEXT: br label [[IF_THEN5]]
; CHECK: if.then5:
-; CHECK-NEXT: [[F1_SROA_PHI:%.*]] = phi ptr [ [[F1_SROA_GEP]], [[IF_THEN2]] ], [ [[F_SROA_0]], [[IF_END]] ]
-; CHECK-NEXT: store i32 0, ptr [[F1_SROA_PHI]], align 4
+; CHECK-NEXT: [[F1:%.*]] = phi ptr [ [[PTR:%.*]], [[IF_THEN2]] ], [ [[F_SROA_0]], [[IF_END]] ]
+; CHECK-NEXT: [[DOTFCA_0_GEP:%.*]] = getelementptr inbounds [[STRUCT_S:%.*]], ptr [[F1]], i32 0, i32 0
+; CHECK-NEXT: store i32 0, ptr [[DOTFCA_0_GEP]], align 4
; CHECK-NEXT: ret void
;
entry:
diff --git a/llvm/test/Transforms/SROA/phi-gep.ll b/llvm/test/Transforms/SROA/phi-gep.ll
index 78071dcdafb49d..c5aa1cdd9cf654 100644
--- a/llvm/test/Transforms/SROA/phi-gep.ll
+++ b/llvm/test/Transforms/SROA/phi-gep.ll
@@ -65,13 +65,15 @@ end:
define i32 @test_sroa_phi_gep_poison(i1 %cond) {
; CHECK-LABEL: @test_sroa_phi_gep_poison(
; CHECK-NEXT: entry:
+; CHECK-NEXT: [[A:%.*]] = alloca [[PAIR:%.*]], align 4
; CHECK-NEXT: br i1 [[COND:%.*]], label [[IF_THEN:%.*]], label [[END:%.*]]
; CHECK: if.then:
-; CHECK-NEXT: [[PHI_SROA_PHI_SROA_SPECULATE_LOAD_IF_THEN:%.*]] = load i32, ptr poison, align 4
; CHECK-NEXT: br label [[END]]
; CHECK: end:
-; CHECK-NEXT: [[PHI_SROA_PHI_SROA_SPECULATED:%.*]] = phi i32 [ undef, [[ENTRY:%.*]] ], [ [[PHI_SROA_PHI_SROA_SPECULATE_LOAD_IF_THEN]], [[IF_THEN]] ]
-; CHECK-NEXT: ret i32 [[PHI_SROA_PHI_SROA_SPECULATED]]
+; CHECK-NEXT: [[PHI:%.*]] = phi ptr [ [[A]], [[ENTRY:%.*]] ], [ poison, [[IF_THEN]] ]
+; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds [[PAIR]], ptr [[PHI]], i32 0, i32 1
+; CHECK-NEXT: [[LOAD:%.*]] = load i32, ptr [[GEP]], align 4
+; CHECK-NEXT: ret i32 [[LOAD]]
;
entry:
%a = alloca %pair, align 4
@@ -92,13 +94,17 @@ end:
define i32 @test_sroa_phi_gep_global(i1 %cond) {
; CHECK-LABEL: @test_sroa_phi_gep_global(
; CHECK-NEXT: entry:
+; CHECK-NEXT: [[A:%.*]] = alloca [[PAIR:%.*]], align 4
+; CHECK-NEXT: [[GEP_A:%.*]] = getelementptr inbounds [[PAIR]], ptr [[A]], i32 0, i32 1
+; CHECK-NEXT: store i32 1, ptr [[GEP_A]], align 4
; CHECK-NEXT: br i1 [[COND:%.*]], label [[IF_THEN:%.*]], label [[END:%.*]]
; CHECK: if.then:
-; CHECK-NEXT: [[PHI_SROA_PHI_SROA_SPECULATE_LOAD_IF_THEN:%.*]] = load i32, ptr getelementptr inbounds ([[PAIR:%.*]], ptr @g, i32 0, i32 1), align 4
; CHECK-NEXT: br label [[END]]
; CHECK: end:
-; CHECK-NEXT: [[PHI_SROA_PHI_SROA_SPECULATED:%.*]] = phi i32 [ 1, [[ENTRY:%.*]] ], [ [[PHI_SROA_PHI_SROA_SPECULATE_LOAD_IF_THEN]], [[IF_THEN]] ]
-; CHECK-NEXT: ret i32 [[PHI_SROA_PHI_SROA_SPECULATED]]
+; CHECK-NEXT: [[PHI:%.*]] = phi ptr [ [[A]], [[ENTRY:%.*]] ], [ @g, [[IF_THEN]] ]
+; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds [[PAIR]], ptr [[PHI]], i32 0, i32 1
+; CHECK-NEXT: [[LOAD:%.*]] = load i32, ptr [[GEP]], align 4
+; CHECK-NEXT: ret i32 [[LOAD]]
;
entry:
%a = alloca %pair, align 4
@@ -239,7 +245,7 @@ define i32 @test_sroa_invoke_phi_gep(i1 %cond) personality ptr @__gxx_personalit
; CHECK-NEXT: br i1 [[COND:%.*]], label [[CALL:%.*]], label [[END:%.*]]
; CHECK: call:
; CHECK-NEXT: [[B:%.*]] = invoke ptr @foo()
-; CHECK-NEXT: to label [[END]] unwind label [[INVOKE_CATCH:%.*]]
+; CHECK-NEXT: to label [[END]] unwind label [[INVOKE_CATCH:%.*]]
; CHECK: end:
; CHECK-NEXT: [[PHI:%.*]] = phi ptr [ [[A]], [[ENTRY:%.*]] ], [ [[B]], [[CALL]] ]
; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds [[PAIR]], ptr [[PHI]], i32 0, i32 1
@@ -247,7 +253,7 @@ define i32 @test_sroa_invoke_phi_gep(i1 %cond) personality ptr @__gxx_personalit
; CHECK-NEXT: ret i32 [[LOAD]]
; CHECK: invoke_catch:
; CHECK-NEXT: [[RES:%.*]] = landingpad { ptr, i32 }
-; CHECK-NEXT: catch ptr null
+; CHECK-NEXT: catch ptr null
; CHECK-NEXT: ret i32 0
;
entry:
@@ -462,10 +468,10 @@ define i32 @test_sroa_phi_gep_multiple_values_from_same_block(i32 %arg) {
; CHECK-LABEL: @test_sroa_phi_gep_multiple_values_from_same_block(
; CHECK-NEXT: bb.1:
; CHECK-NEXT: switch i32 [[ARG:%.*]], label [[BB_3:%.*]] [
-; CHECK-NEXT: i32 1, label [[BB_2:%.*]]
-; CHECK-NEXT: i32 2, label [[BB_2]]
-; CHECK-NEXT: i32 3, label [[BB_4:%.*]]
-; CHECK-NEXT: i32 4, label [[BB_4]]
+; CHECK-NEXT: i32 1, label [[BB_2:%.*]]
+; CHECK-NEXT: i32 2, label [[BB_2]]
+; CHECK-NEXT: i32 3, label [[BB_4:%.*]]
+; CHECK-NEXT: i32 4, label [[BB_4]]
; CHECK-NEXT: ]
; CHECK: bb.2:
; CHECK-NEXT: br label [[BB_4]]
@@ -498,117 +504,6 @@ bb.4: ; preds = %bb.1, %bb.1, %bb
ret i32 %load
}
-define i64 @test_phi_idx_mem2reg_const(i1 %arg) {
-; CHECK-LABEL: @test_phi_idx_mem2reg_const(
-; CHECK-NEXT: bb:
-; CHECK-NEXT: br i1 [[ARG:%.*]], label [[BB1:%.*]], label [[BB2:%.*]]
-; CHECK: bb1:
-; CHECK-NEXT: br label [[END:%.*]]
-; CHECK: bb2:
-; CHECK-NEXT: br label [[END]]
-; CHECK: end:
-; CHECK-NEXT: [[PHI_SROA_PHI_SROA_SPECULATED:%.*]] = phi i64 [ 2, [[BB1]] ], [ 3, [[BB2]] ]
-; CHECK-NEXT: [[PHI:%.*]] = phi i64 [ 0, [[BB1]] ], [ 1, [[BB2]] ]
-; CHECK-NEXT: ret i64 [[PHI_SROA_PHI_SROA_SPECULATED]]
-;
-bb:
- %alloca = alloca [2 x i64], align 8
- %gep1 = getelementptr inbounds i64, ptr %alloca, i64 1
- store i64 2, ptr %alloca
- store i64 3, ptr %gep1
- br i1 %arg, label %bb1, label %bb2
-
-bb1:
- br label %end
-
-bb2:
- br label %end
-
-end:
- %phi = phi i64 [ 0, %bb1 ], [ 1, %bb2 ]
- %getelementptr = getelementptr inbounds i64, ptr %alloca, i64 %phi
- %load = load i64, ptr %getelementptr
- ret i64 %load
-}
-
-define i64 @test_phi_idx_mem2reg_not_const(i1 %arg, i64 %idx) {
-; CHECK-LABEL: @test_phi_idx_mem2reg_not_const(
-; CHECK-NEXT: bb:
-; CHECK-NEXT: [[ALLOCA:%.*]] = alloca [2 x i64], align 8
-; CHECK-NEXT: [[GEP1:%.*]] = getelementptr inbounds i64, ptr [[ALLOCA]], i64 1
-; CHECK-NEXT: store i64 2, ptr [[ALLOCA]], align 4
-; CHECK-NEXT: store i64 3, ptr [[GEP1]], align 4
-; CHECK-NEXT: br i1 [[ARG:%.*]], label [[BB1:%.*]], label [[BB2:%.*]]
-; CHECK: bb1:
-; CHECK-NEXT: br label [[END:%.*]]
-; CHECK: bb2:
-; CHECK-NEXT: br label [[END]]
-; CHECK: end:
-; CHECK-NEXT: [[PHI:%.*]] = phi i64 [ 0, [[BB1]] ], [ [[IDX:%.*]], [[BB2]] ]
-; CHECK-NEXT: [[GETELEMENTPTR:%.*]] = getelementptr inbounds i64, ptr [[ALLOCA]], i64 [[PHI]]
-; CHECK-NEXT: [[LOAD:%.*]] = load i64, ptr [[GETELEMENTPTR]], align 4
-; CHECK-NEXT: ret i64 [[LOAD]]
-;
-bb:
- %alloca = alloca [2 x i64], align 8
- %gep1 = getelementptr inbounds i64, ptr %alloca, i64 1
- store i64 2, ptr %alloca
- store i64 3, ptr %gep1
- br i1 %arg, label %bb1, label %bb2
-
-bb1:
- br label %end
-
-bb2:
- br label %end
-
-end:
- %phi = phi i64 [ 0, %bb1 ], [ %idx, %bb2 ]
- %getelementptr = getelementptr inbounds i64, ptr %alloca, i64 %phi
- %load = load i64, ptr %getelementptr
- ret i64 %load
-}
-
-define i64 @test_phi_mem2reg_pointer_op_is_non_const_gep(i1 %arg, i64 %idx) {
-; CHECK-LABEL: @test_phi_mem2reg_pointer_op_is_non_const_gep(
-; CHECK-NEXT: bb:
-; CHECK-NEXT: [[ALLOCA:%.*]] = alloca [2 x i64], align 8
-; CHECK-NEXT: [[GEP1:%.*]] = getelementptr inbounds i64, ptr [[ALLOCA]], i64 1
-; CHECK-NEXT: store i64 2, ptr [[ALLOCA]], align 4
-; CHECK-NEXT: store i64 3, ptr [[GEP1]], align 4
-; CHECK-NEXT: br i1 [[ARG:%.*]], label [[BB1:%.*]], label [[BB2:%.*]]
-; CHECK: bb1:
-; CHECK-NEXT: br label [[END:%.*]]
-; CHECK: bb2:
-; CHECK-NEXT: br label [[END]]
-; CHECK: end:
-; CHECK-NEXT: [[PHI:%.*]] = phi i64 [ 0, [[BB1]] ], [ 1, [[BB2]] ]
-; CHECK-NEXT: [[GETELEMENTPTR:%.*]] = getelementptr inbounds i64, ptr [[ALLOCA]], i64 [[IDX:%.*]]
-; CHECK-NEXT: [[GETELEMENTPTR2:%.*]] = getelementptr inbounds i64, ptr [[GETELEMENTPTR]], i64 [[PHI]]
-; CHECK-NEXT: [[LOAD:%.*]] = load i64, ptr [[GETELEMENTPTR]], align 4
-; CHECK-NEXT: ret i64 [[LOAD]]
-;
-bb:
- %alloca = alloca [2 x i64], align 8
- %gep1 = getelementptr inbounds i64, ptr %alloca, i64 1
- store i64 2, ptr %alloca
- store i64 3, ptr %gep1
- br i1 %arg, label %bb1, label %bb2
-
-bb1:
- br label %end
-
-bb2:
- br label %end
-
-end:
- %phi = phi i64 [ 0, %bb1 ], [ 1, %bb2 ]
- %getelementptr = getelementptr inbounds i64, ptr %alloca, i64 %idx
- %getelementptr2 = getelementptr inbounds i64, ptr %getelementptr, i64 %phi
- %load = load i64, ptr %getelementptr
- ret i64 %load
-}
-
declare ptr @foo()
declare i32 @__gxx_personality_v0(...)
More information about the llvm-commits
mailing list