[llvm] 8eded24 - Recommit "[SCEVExpander] Add helper to clean up instrs inserted while expanding."
Florian Hahn via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 21 07:06:49 PDT 2020
Author: Florian Hahn
Date: 2020-08-21T15:04:17+01:00
New Revision: 8eded24bf46c05ffd110d521f58320cdee93866e
URL: https://github.com/llvm/llvm-project/commit/8eded24bf46c05ffd110d521f58320cdee93866e
DIFF: https://github.com/llvm/llvm-project/commit/8eded24bf46c05ffd110d521f58320cdee93866e.diff
LOG: Recommit "[SCEVExpander] Add helper to clean up instrs inserted while expanding."
Recommit the patch after fixing an issue reported caused by the fact
that re-used values are also added to InsertedValues.
Additional tests have been added in 88818491b9dea64ec65c92ce5652bc45bef337a4
This reverts the revert commit 38884641f28e373ce291dc5ea93416756216e536.
Added:
Modified:
llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h b/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
index b6e6f0d780fe..78ae38288c0c 100644
--- a/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
+++ b/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
@@ -63,6 +63,11 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
DenseSet<AssertingVH<Value>> InsertedValues;
DenseSet<AssertingVH<Value>> InsertedPostIncValues;
+ /// Keep track of the existing IR values re-used during expansion.
+ /// FIXME: Ideally re-used instructions would not be added to
+ /// InsertedValues/InsertedPostIncValues.
+ SmallPtrSet<Value *, 16> ReusedValues;
+
/// A memoization of the "relevant" loop for a given SCEV.
DenseMap<const SCEV *, const Loop *> RelevantLoops;
@@ -177,9 +182,31 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
InsertedExpressions.clear();
InsertedValues.clear();
InsertedPostIncValues.clear();
+ ReusedValues.clear();
ChainedPhis.clear();
}
+ /// Return a vector containing all instructions inserted during expansion.
+ SmallVector<Instruction *, 32> getAllInsertedInstructions() const {
+ SmallVector<Instruction *, 32> Result;
+ for (auto &VH : InsertedValues) {
+ Value *V = VH;
+ if (ReusedValues.contains(V))
+ continue;
+ if (auto *Inst = dyn_cast<Instruction>(V))
+ Result.push_back(Inst);
+ }
+ for (auto &VH : InsertedPostIncValues) {
+ Value *V = VH;
+ if (ReusedValues.contains(V))
+ continue;
+ if (auto *Inst = dyn_cast<Instruction>(V))
+ Result.push_back(Inst);
+ }
+
+ return Result;
+ }
+
/// Return true for expressions that can't be evaluated at runtime
/// within given \b Budget.
///
@@ -317,7 +344,8 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
}
/// Return true if the specified instruction was inserted by the code
- /// rewriter. If so, the client should not modify the instruction.
+ /// rewriter. If so, the client should not modify the instruction. Note that
+ /// this also includes instructions re-used during expansion.
bool isInsertedInstruction(Instruction *I) const {
return InsertedValues.count(I) || InsertedPostIncValues.count(I);
}
@@ -452,6 +480,27 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
/// If no PHIs have been created, return the unchanged operand \p OpIdx.
Value *fixupLCSSAFormFor(Instruction *User, unsigned OpIdx);
};
+
+/// Helper to remove instructions inserted during SCEV expansion, unless they
+/// are marked as used.
+class SCEVExpanderCleaner {
+ SCEVExpander &Expander;
+
+ DominatorTree &DT;
+
+ /// Indicates whether the result of the expansion is used. If false, the
+ /// instructions added during expansion are removed.
+ bool ResultUsed;
+
+public:
+ SCEVExpanderCleaner(SCEVExpander &Expander, DominatorTree &DT)
+ : Expander(Expander), DT(DT), ResultUsed(false) {}
+
+ ~SCEVExpanderCleaner();
+
+ /// Indicate that the result of the expansion is used.
+ void markResultUsed() { ResultUsed = true; }
+};
} // namespace llvm
#endif
diff --git a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
index 52993c9dcb09..d0b6244efcce 100644
--- a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
@@ -295,31 +295,6 @@ static void deleteDeadInstruction(Instruction *I) {
I->eraseFromParent();
}
-namespace {
-class ExpandedValuesCleaner {
- SCEVExpander &Expander;
- TargetLibraryInfo *TLI;
- SmallVector<Value *, 4> ExpandedValues;
- bool Commit = false;
-
-public:
- ExpandedValuesCleaner(SCEVExpander &Expander, TargetLibraryInfo *TLI)
- : Expander(Expander), TLI(TLI) {}
-
- void add(Value *V) { ExpandedValues.push_back(V); }
-
- void commit() { Commit = true; }
-
- ~ExpandedValuesCleaner() {
- if (!Commit) {
- Expander.clear();
- for (auto *V : ExpandedValues)
- RecursivelyDeleteTriviallyDeadInstructions(V, TLI);
- }
- }
-};
-} // namespace
-
//===----------------------------------------------------------------------===//
//
// Implementation of LoopIdiomRecognize
@@ -933,7 +908,7 @@ bool LoopIdiomRecognize::processLoopStridedStore(
BasicBlock *Preheader = CurLoop->getLoopPreheader();
IRBuilder<> Builder(Preheader->getTerminator());
SCEVExpander Expander(*SE, *DL, "loop-idiom");
- ExpandedValuesCleaner EVC(Expander, TLI);
+ SCEVExpanderCleaner ExpCleaner(Expander, *DT);
Type *DestInt8PtrTy = Builder.getInt8PtrTy(DestAS);
Type *IntIdxTy = DL->getIndexType(DestPtr->getType());
@@ -956,7 +931,6 @@ bool LoopIdiomRecognize::processLoopStridedStore(
// base pointer and checking the region.
Value *BasePtr =
Expander.expandCodeFor(Start, DestInt8PtrTy, Preheader->getTerminator());
- EVC.add(BasePtr);
// From here on out, conservatively report to the pass manager that we've
// changed the IR, even if we later clean up these added instructions. There
@@ -1041,7 +1015,7 @@ bool LoopIdiomRecognize::processLoopStridedStore(
if (MSSAU && VerifyMemorySSA)
MSSAU->getMemorySSA()->verifyMemorySSA();
++NumMemSet;
- EVC.commit();
+ ExpCleaner.markResultUsed();
return true;
}
@@ -1075,7 +1049,7 @@ bool LoopIdiomRecognize::processLoopStoreOfLoopLoad(StoreInst *SI,
IRBuilder<> Builder(Preheader->getTerminator());
SCEVExpander Expander(*SE, *DL, "loop-idiom");
- ExpandedValuesCleaner EVC(Expander, TLI);
+ SCEVExpanderCleaner ExpCleaner(Expander, *DT);
bool Changed = false;
const SCEV *StrStart = StoreEv->getStart();
@@ -1094,7 +1068,6 @@ bool LoopIdiomRecognize::processLoopStoreOfLoopLoad(StoreInst *SI,
// checking everything.
Value *StoreBasePtr = Expander.expandCodeFor(
StrStart, Builder.getInt8PtrTy(StrAS), Preheader->getTerminator());
- EVC.add(StoreBasePtr);
// From here on out, conservatively report to the pass manager that we've
// changed the IR, even if we later clean up these added instructions. There
@@ -1122,7 +1095,6 @@ bool LoopIdiomRecognize::processLoopStoreOfLoopLoad(StoreInst *SI,
// mutated by the loop.
Value *LoadBasePtr = Expander.expandCodeFor(
LdStart, Builder.getInt8PtrTy(LdAS), Preheader->getTerminator());
- EVC.add(LoadBasePtr);
if (mayLoopAccessLocation(LoadBasePtr, ModRefInfo::Mod, CurLoop, BECount,
StoreSize, *AA, Stores))
@@ -1138,7 +1110,6 @@ bool LoopIdiomRecognize::processLoopStoreOfLoopLoad(StoreInst *SI,
Value *NumBytes =
Expander.expandCodeFor(NumBytesS, IntIdxTy, Preheader->getTerminator());
- EVC.add(NumBytes);
CallInst *NewCall = nullptr;
// Check whether to generate an unordered atomic memcpy:
@@ -1198,7 +1169,7 @@ bool LoopIdiomRecognize::processLoopStoreOfLoopLoad(StoreInst *SI,
if (MSSAU && VerifyMemorySSA)
MSSAU->getMemorySSA()->verifyMemorySSA();
++NumMemCpy;
- EVC.commit();
+ ExpCleaner.markResultUsed();
return true;
}
diff --git a/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp b/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
index da6a719dccd1..1e8b11d6ac5f 100644
--- a/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
+++ b/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
@@ -1258,6 +1258,9 @@ SCEVExpander::getAddRecExprPHILiterally(const SCEVAddRecExpr *Normalized,
InsertedValues.insert(AddRecPhiMatch);
// Remember the increment.
rememberInstruction(IncV);
+ // Those values were not actually inserted but re-used.
+ ReusedValues.insert(AddRecPhiMatch);
+ ReusedValues.insert(IncV);
return AddRecPhiMatch;
}
}
@@ -1882,10 +1885,12 @@ Value *SCEVExpander::expand(const SCEV *S) {
// there) so that it is guaranteed to dominate any user inside the loop.
if (L && SE.hasComputableLoopEvolution(S, L) && !PostIncLoops.count(L))
InsertPt = &*L->getHeader()->getFirstInsertionPt();
+
while (InsertPt->getIterator() != Builder.GetInsertPoint() &&
(isInsertedInstruction(InsertPt) ||
- isa<DbgInfoIntrinsic>(InsertPt)))
+ isa<DbgInfoIntrinsic>(InsertPt))) {
InsertPt = &*std::next(InsertPt->getIterator());
+ }
break;
}
}
@@ -2630,4 +2635,40 @@ bool isSafeToExpandAt(const SCEV *S, const Instruction *InsertionPoint,
}
return false;
}
+
+SCEVExpanderCleaner::~SCEVExpanderCleaner() {
+ // Result is used, nothing to remove.
+ if (ResultUsed)
+ return;
+
+ auto InsertedInstructions = Expander.getAllInsertedInstructions();
+#ifndef NDEBUG
+ SmallPtrSet<Instruction *, 8> InsertedSet(InsertedInstructions.begin(),
+ InsertedInstructions.end());
+ (void)InsertedSet;
+#endif
+ // Remove sets with value handles.
+ Expander.clear();
+
+ // Sort so that earlier instructions do not dominate later instructions.
+ stable_sort(InsertedInstructions, [this](Instruction *A, Instruction *B) {
+ return DT.dominates(B, A);
+ });
+ // Remove all inserted instructions.
+ for (Instruction *I : InsertedInstructions) {
+
+#ifndef NDEBUG
+ assert(all_of(I->users(),
+ [&InsertedSet](Value *U) {
+ return InsertedSet.contains(cast<Instruction>(U));
+ }) &&
+ "removed instruction should only be used by instructions inserted "
+ "during expansion");
+#endif
+ assert(!I->getType()->isVoidTy() &&
+ "inserted instruction should have non-void types");
+ I->replaceAllUsesWith(UndefValue::get(I->getType()));
+ I->eraseFromParent();
+ }
+}
}
More information about the llvm-commits
mailing list