[llvm] 7b0d59d - [IndVars] Drop check for the validity of rewrite
Roman Lebedev via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 30 02:12:10 PDT 2021
Author: Roman Lebedev
Date: 2021-08-30T12:06:58+03:00
New Revision: 7b0d59da9af4bf4eb8342cac579e42a818ac1ae7
URL: https://github.com/llvm/llvm-project/commit/7b0d59da9af4bf4eb8342cac579e42a818ac1ae7
DIFF: https://github.com/llvm/llvm-project/commit/7b0d59da9af4bf4eb8342cac579e42a818ac1ae7.diff
LOG: [IndVars] Drop check for the validity of rewrite
`isValidRewrite()` checks that the both the original SCEV,
and the rewrite SCEV have the same base pointer.
I //believe//, after all the recent SCEV improvements,
this invariant is already enforced by SCEV itself.
I originally tried changing it into an assert in D108043,
but that showed that it triggers on e.g. https://reviews.llvm.org/D108043#2946621,
where SCEV manages to forward the store to load,
test added.
Reviewed By: nikic
Differential Revision: https://reviews.llvm.org/D108655
Added:
Modified:
llvm/lib/Transforms/Utils/LoopUtils.cpp
llvm/test/Transforms/IndVarSimplify/D108043.ll
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/Utils/LoopUtils.cpp b/llvm/lib/Transforms/Utils/LoopUtils.cpp
index b82b95acb4360..25d22f189a6ef 100644
--- a/llvm/lib/Transforms/Utils/LoopUtils.cpp
+++ b/llvm/lib/Transforms/Utils/LoopUtils.cpp
@@ -1110,58 +1110,6 @@ bool llvm::cannotBeMaxInLoop(const SCEV *S, const Loop *L, ScalarEvolution &SE,
// As a side effect, reduces the amount of IV processing within the loop.
//===----------------------------------------------------------------------===//
-// Return true if the SCEV expansion generated by the rewriter can replace the
-// original value. SCEV guarantees that it produces the same value, but the way
-// it is produced may be illegal IR. Ideally, this function will only be
-// called for verification.
-static bool isValidRewrite(ScalarEvolution *SE, Value *FromVal, Value *ToVal) {
- // If an SCEV expression subsumed multiple pointers, its expansion could
- // reassociate the GEP changing the base pointer. This is illegal because the
- // final address produced by a GEP chain must be inbounds relative to its
- // underlying object. Otherwise basic alias analysis, among other things,
- // could fail in a dangerous way. Ultimately, SCEV will be improved to avoid
- // producing an expression involving multiple pointers. Until then, we must
- // bail out here.
- //
- // Retrieve the pointer operand of the GEP. Don't use getUnderlyingObject
- // because it understands lcssa phis while SCEV does not.
- Value *FromPtr = FromVal;
- Value *ToPtr = ToVal;
- if (auto *GEP = dyn_cast<GEPOperator>(FromVal))
- FromPtr = GEP->getPointerOperand();
-
- if (auto *GEP = dyn_cast<GEPOperator>(ToVal))
- ToPtr = GEP->getPointerOperand();
-
- if (FromPtr != FromVal || ToPtr != ToVal) {
- // Quickly check the common case
- if (FromPtr == ToPtr)
- return true;
-
- // SCEV may have rewritten an expression that produces the GEP's pointer
- // operand. That's ok as long as the pointer operand has the same base
- // pointer. Unlike getUnderlyingObject(), getPointerBase() will find the
- // base of a recurrence. This handles the case in which SCEV expansion
- // converts a pointer type recurrence into a nonrecurrent pointer base
- // indexed by an integer recurrence.
-
- // If the GEP base pointer is a vector of pointers, abort.
- if (!FromPtr->getType()->isPointerTy() || !ToPtr->getType()->isPointerTy())
- return false;
-
- const SCEV *FromBase = SE->getPointerBase(SE->getSCEV(FromPtr));
- const SCEV *ToBase = SE->getPointerBase(SE->getSCEV(ToPtr));
- if (FromBase == ToBase)
- return true;
-
- LLVM_DEBUG(dbgs() << "rewriteLoopExitValues: GEP rewrite bail out "
- << *FromBase << " != " << *ToBase << "\n");
-
- return false;
- }
- return true;
-}
-
static bool hasHardUserWithinLoop(const Loop *L, const Instruction *I) {
SmallPtrSet<const Instruction *, 8> Visited;
SmallVector<const Instruction *, 8> WorkList;
@@ -1195,7 +1143,6 @@ struct RewritePhi {
bool HighCost; // Is this expansion a high-cost?
Value *Expansion = nullptr;
- bool ValidRewrite = false;
RewritePhi(PHINode *P, unsigned I, const SCEV *Val, Instruction *ExpansionPt,
bool H)
@@ -1233,8 +1180,6 @@ static bool canLoopBeDeleted(Loop *L, SmallVector<RewritePhi, 8> &RewritePhiSet)
// phase later. Skip it in the loop invariant check below.
bool found = false;
for (const RewritePhi &Phi : RewritePhiSet) {
- if (!Phi.ValidRewrite)
- continue;
unsigned i = Phi.Ith;
if (Phi.PN == P && (Phi.PN)->getIncomingValue(i) == Incoming) {
found = true;
@@ -1378,13 +1323,6 @@ int llvm::rewriteLoopExitValues(Loop *L, LoopInfo *LI, TargetLibraryInfo *TLI,
<< *(Phi.Expansion) << '\n'
<< " LoopVal = " << *(Phi.ExpansionPoint) << "\n");
- // FIXME: isValidRewrite() is a hack. it should be an assert, eventually.
- Phi.ValidRewrite = isValidRewrite(SE, Phi.ExpansionPoint, Phi.Expansion);
- if (!Phi.ValidRewrite) {
- DeadInsts.push_back(Phi.Expansion);
- continue;
- }
-
#ifndef NDEBUG
// If we reuse an instruction from a loop which is neither L nor one of
// its containing loops, we end up breaking LCSSA form for this loop by
@@ -1407,9 +1345,6 @@ int llvm::rewriteLoopExitValues(Loop *L, LoopInfo *LI, TargetLibraryInfo *TLI,
// Transformation.
for (const RewritePhi &Phi : RewritePhiSet) {
- if (!Phi.ValidRewrite)
- continue;
-
PHINode *PN = Phi.PN;
Value *ExitVal = Phi.Expansion;
diff --git a/llvm/test/Transforms/IndVarSimplify/D108043.ll b/llvm/test/Transforms/IndVarSimplify/D108043.ll
index d12506b1701b5..796342c24ba5d 100644
--- a/llvm/test/Transforms/IndVarSimplify/D108043.ll
+++ b/llvm/test/Transforms/IndVarSimplify/D108043.ll
@@ -9,17 +9,12 @@ define internal fastcc void @func_2() unnamed_addr {
; CHECK-NEXT: lbl_2898.preheader:
; CHECK-NEXT: br label [[LBL_2898:%.*]]
; CHECK: lbl_2898.loopexit:
-; CHECK-NEXT: [[DOTLCSSA:%.*]] = phi i32* [ [[TMP0:%.*]], [[FOR_COND884:%.*]] ]
-; CHECK-NEXT: store i32* [[DOTLCSSA]], i32** @g_1150, align 8
+; CHECK-NEXT: store i32* getelementptr inbounds ([4 x [6 x i32]], [4 x [6 x i32]]* @g_2168, i64 0, i64 3, i64 1), i32** @g_1150, align 8
; CHECK-NEXT: br label [[LBL_2898]]
; CHECK: lbl_2898:
-; CHECK-NEXT: [[G_1150_PROMOTED:%.*]] = load i32*, i32** @g_1150, align 8
-; CHECK-NEXT: br label [[FOR_COND884]]
+; CHECK-NEXT: br label [[FOR_COND884:%.*]]
; CHECK: for.cond884:
-; CHECK-NEXT: [[TMP0]] = phi i32* [ getelementptr inbounds ([4 x [6 x i32]], [4 x [6 x i32]]* @g_2168, i64 0, i64 3, i64 1), [[FOR_END987:%.*]] ], [ [[G_1150_PROMOTED]], [[LBL_2898]] ]
-; CHECK-NEXT: [[STOREMERGE9:%.*]] = phi i16 [ [[ADD990:%.*]], [[FOR_END987]] ], [ 0, [[LBL_2898]] ]
-; CHECK-NEXT: [[CMP886:%.*]] = icmp ult i16 [[STOREMERGE9]], 3
-; CHECK-NEXT: br i1 [[CMP886]], label [[FOR_BODY888:%.*]], label [[LBL_2898_LOOPEXIT:%.*]]
+; CHECK-NEXT: br i1 false, label [[FOR_BODY888:%.*]], label [[LBL_2898_LOOPEXIT:%.*]]
; CHECK: for.body888:
; CHECK-NEXT: br label [[FOR_COND918:%.*]]
; CHECK: for.cond918:
@@ -27,9 +22,8 @@ define internal fastcc void @func_2() unnamed_addr {
; CHECK: for.end926:
; CHECK-NEXT: br label [[FOR_COND936:%.*]]
; CHECK: for.cond936:
-; CHECK-NEXT: br label [[FOR_END987]]
+; CHECK-NEXT: br label [[FOR_END987:%.*]]
; CHECK: for.end987:
-; CHECK-NEXT: [[ADD990]] = add nuw nsw i16 [[STOREMERGE9]], 1
; CHECK-NEXT: br label [[FOR_COND884]]
;
lbl_2898.preheader:
More information about the llvm-commits
mailing list