Transforms: Have CSE/GVN/LICM check isProfitableToHoist() before hoisting. #141325
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.6-beta.1
|
@llvm/pr-subscribers-llvm-transforms Author: Peter Collingbourne (pcc) ChangesLICM hoists instructions into the preheader, and CSE and GVN This will be tested in the subsequent change that adds a previously Full diff: https://github.com/llvm/llvm-project/pull/141325.diff 5 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Scalar/GVN.h b/llvm/include/llvm/Transforms/Scalar/GVN.h
index 25b042eebf664..b7a2ced3a7a21 100644
--- a/llvm/include/llvm/Transforms/Scalar/GVN.h
+++ b/llvm/include/llvm/Transforms/Scalar/GVN.h
@@ -53,6 +53,7 @@ class NonLocalDepResult;
class OptimizationRemarkEmitter;
class PHINode;
class TargetLibraryInfo;
+class TargetTransformInfo;
class Value;
/// A private "module" namespace for types and utilities used by GVN. These
/// are implementation details and should not be used by clients.
@@ -226,6 +227,7 @@ class GVNPass : public PassInfoMixin<GVNPass> {
MemoryDependenceResults *MD = nullptr;
DominatorTree *DT = nullptr;
const TargetLibraryInfo *TLI = nullptr;
+ const TargetTransformInfo *TTI = nullptr;
AssumptionCache *AC = nullptr;
SetVector<BasicBlock *> DeadBlocks;
OptimizationRemarkEmitter *ORE = nullptr;
@@ -318,7 +320,8 @@ class GVNPass : public PassInfoMixin<GVNPass> {
using UnavailBlkVect = SmallVector<BasicBlock *, 64>;
bool runImpl(Function &F, AssumptionCache &RunAC, DominatorTree &RunDT,
- const TargetLibraryInfo &RunTLI, AAResults &RunAA,
+ const TargetLibraryInfo &RunTLI,
+ const TargetTransformInfo &RunTTI, AAResults &RunAA,
MemoryDependenceResults *RunMD, LoopInfo &LI,
OptimizationRemarkEmitter *ORE, MemorySSA *MSSA = nullptr);
diff --git a/llvm/include/llvm/Transforms/Utils/LoopUtils.h b/llvm/include/llvm/Transforms/Utils/LoopUtils.h
index 416a0a70325d1..4fa8445ce5cc8 100644
--- a/llvm/include/llvm/Transforms/Utils/LoopUtils.h
+++ b/llvm/include/llvm/Transforms/Utils/LoopUtils.h
@@ -175,10 +175,10 @@ bool sinkRegionForLoopNest(DomTreeNode *, AAResults *, LoopInfo *,
/// \p AllowSpeculation is whether values should be hoisted even if they are not
/// guaranteed to execute in the loop, but are safe to speculatively execute.
bool hoistRegion(DomTreeNode *, AAResults *, LoopInfo *, DominatorTree *,
- AssumptionCache *, TargetLibraryInfo *, Loop *,
- MemorySSAUpdater &, ScalarEvolution *, ICFLoopSafetyInfo *,
- SinkAndHoistLICMFlags &, OptimizationRemarkEmitter *, bool,
- bool AllowSpeculation);
+ AssumptionCache *, TargetLibraryInfo *, TargetTransformInfo *,
+ Loop *, MemorySSAUpdater &, ScalarEvolution *,
+ ICFLoopSafetyInfo *, SinkAndHoistLICMFlags &,
+ OptimizationRemarkEmitter *, bool, bool AllowSpeculation);
/// Return true if the induction variable \p IV in a Loop whose latch is
/// \p LatchBlock would become dead if the exit test \p Cond were removed.
diff --git a/llvm/lib/Transforms/Scalar/EarlyCSE.cpp b/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
index 1e378369166c0..16faf72c488fe 100644
--- a/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
+++ b/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
@@ -1534,6 +1534,16 @@ bool EarlyCSE::processNode(DomTreeNode *Node) {
}
// See if the instruction has an available value. If so, use it.
if (Value *V = AvailableValues.lookup(&Inst)) {
+ // If this CSE would effectively hoist the instruction and that's known
+ // to be unprofitable, don't do it. Remember the instruction so that it
+ // can be used by other instructions in the same block.
+ if (auto *ReplI = dyn_cast<Instruction>(V)) {
+ if (ReplI->getParent() != Inst.getParent() &&
+ !TTI.isProfitableToHoist(&Inst)) {
+ AvailableValues.insert(&Inst, &Inst);
+ continue;
+ }
+ }
LLVM_DEBUG(dbgs() << "EarlyCSE CSE: " << Inst << " to: " << *V
<< '\n');
if (!DebugCounter::shouldExecute(CSECounter)) {
diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index 77ca14f88a834..0cb38bcfff2d4 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -42,6 +42,7 @@
#include "llvm/Analysis/OptimizationRemarkEmitter.h"
#include "llvm/Analysis/PHITransAddr.h"
#include "llvm/Analysis/TargetLibraryInfo.h"
+#include "llvm/Analysis/TargetTransformInfo.h"
#include "llvm/Analysis/ValueTracking.h"
#include "llvm/IR/Attributes.h"
#include "llvm/IR/BasicBlock.h"
@@ -832,6 +833,7 @@ PreservedAnalyses GVNPass::run(Function &F, FunctionAnalysisManager &AM) {
auto &AC = AM.getResult<AssumptionAnalysis>(F);
auto &DT = AM.getResult<DominatorTreeAnalysis>(F);
auto &TLI = AM.getResult<TargetLibraryAnalysis>(F);
+ auto &TTI = AM.getResult<TargetIRAnalysis>(F);
auto &AA = AM.getResult<AAManager>(F);
auto *MemDep =
isMemDepEnabled() ? &AM.getResult<MemoryDependenceAnalysis>(F) : nullptr;
@@ -843,7 +845,7 @@ PreservedAnalyses GVNPass::run(Function &F, FunctionAnalysisManager &AM) {
MSSA = &AM.getResult<MemorySSAAnalysis>(F);
}
auto &ORE = AM.getResult<OptimizationRemarkEmitterAnalysis>(F);
- bool Changed = runImpl(F, AC, DT, TLI, AA, MemDep, LI, &ORE,
+ bool Changed = runImpl(F, AC, DT, TLI, TTI, AA, MemDep, LI, &ORE,
MSSA ? &MSSA->getMSSA() : nullptr);
if (!Changed)
return PreservedAnalyses::all();
@@ -2719,6 +2721,16 @@ bool GVNPass::processInstruction(Instruction *I) {
return false;
}
+ // If this GVN would effectively hoist the instruction and that's known to be
+ // unprofitable, don't do it. Remember the instruction so that it can be used
+ // by other instructions in the same block.
+ if (auto *ReplI = dyn_cast<Instruction>(I)) {
+ if (ReplI->getParent() != I->getParent() && !TTI->isProfitableToHoist(I)) {
+ LeaderTable.insert(Num, I, I->getParent());
+ return false;
+ }
+ }
+
// Remove it!
patchAndReplaceAllUsesWith(I, Repl);
if (MD && Repl->getType()->isPtrOrPtrVectorTy())
@@ -2729,13 +2741,15 @@ bool GVNPass::processInstruction(Instruction *I) {
/// runOnFunction - This is the main transformation entry point for a function.
bool GVNPass::runImpl(Function &F, AssumptionCache &RunAC, DominatorTree &RunDT,
- const TargetLibraryInfo &RunTLI, AAResults &RunAA,
+ const TargetLibraryInfo &RunTLI,
+ const TargetTransformInfo &RunTTI, AAResults &RunAA,
MemoryDependenceResults *RunMD, LoopInfo &LI,
OptimizationRemarkEmitter *RunORE, MemorySSA *MSSA) {
AC = &RunAC;
DT = &RunDT;
VN.setDomTree(DT);
TLI = &RunTLI;
+ TTI = &RunTTI;
VN.setAliasAnalysis(&RunAA);
MD = RunMD;
ImplicitControlFlowTracking ImplicitCFT;
@@ -3295,6 +3309,7 @@ class llvm::gvn::GVNLegacyPass : public FunctionPass {
F, getAnalysis<AssumptionCacheTracker>().getAssumptionCache(F),
getAnalysis<DominatorTreeWrapperPass>().getDomTree(),
getAnalysis<TargetLibraryInfoWrapperPass>().getTLI(F),
+ getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F),
getAnalysis<AAResultsWrapperPass>().getAAResults(),
Impl.isMemDepEnabled()
? &getAnalysis<MemoryDependenceWrapperPass>().getMemDep()
@@ -3308,6 +3323,7 @@ class llvm::gvn::GVNLegacyPass : public FunctionPass {
AU.addRequired<AssumptionCacheTracker>();
AU.addRequired<DominatorTreeWrapperPass>();
AU.addRequired<TargetLibraryInfoWrapperPass>();
+ AU.addRequired<TargetTransformInfoWrapperPass>();
AU.addRequired<LoopInfoWrapperPass>();
if (Impl.isMemDepEnabled())
AU.addRequired<MemoryDependenceWrapperPass>();
diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index 62ef40c686874..ae5c4387a49c3 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -467,9 +467,9 @@ bool LoopInvariantCodeMotion::runOnLoop(Loop *L, AAResults *AA, LoopInfo *LI,
MSSAU, &SafetyInfo, Flags, ORE);
Flags.setIsSink(false);
if (Preheader)
- Changed |= hoistRegion(DT->getNode(L->getHeader()), AA, LI, DT, AC, TLI, L,
- MSSAU, SE, &SafetyInfo, Flags, ORE, LoopNestMode,
- LicmAllowSpeculation);
+ Changed |= hoistRegion(DT->getNode(L->getHeader()), AA, LI, DT, AC, TLI,
+ TTI, L, MSSAU, SE, &SafetyInfo, Flags, ORE,
+ LoopNestMode, LicmAllowSpeculation);
// Now that all loop invariants have been removed from the loop, promote any
// memory references to scalars that we can.
@@ -873,9 +873,9 @@ class ControlFlowHoister {
///
bool llvm::hoistRegion(DomTreeNode *N, AAResults *AA, LoopInfo *LI,
DominatorTree *DT, AssumptionCache *AC,
- TargetLibraryInfo *TLI, Loop *CurLoop,
- MemorySSAUpdater &MSSAU, ScalarEvolution *SE,
- ICFLoopSafetyInfo *SafetyInfo,
+ TargetLibraryInfo *TLI, TargetTransformInfo *TTI,
+ Loop *CurLoop, MemorySSAUpdater &MSSAU,
+ ScalarEvolution *SE, ICFLoopSafetyInfo *SafetyInfo,
SinkAndHoistLICMFlags &Flags,
OptimizationRemarkEmitter *ORE, bool LoopNestMode,
bool AllowSpeculation) {
@@ -911,11 +911,12 @@ bool llvm::hoistRegion(DomTreeNode *N, AAResults *AA, LoopInfo *LI,
// TODO: It may be safe to hoist if we are hoisting to a conditional block
// and we have accurately duplicated the control flow from the loop header
// to that block.
- if (CurLoop->hasLoopInvariantOperands(&I) &&
+ if (TTI->isProfitableToHoist(&I) &&
+ CurLoop->hasLoopInvariantOperands(&I) &&
canSinkOrHoistInst(I, AA, DT, CurLoop, MSSAU, true, Flags, ORE) &&
- isSafeToExecuteUnconditionally(
- I, DT, TLI, CurLoop, SafetyInfo, ORE,
- Preheader->getTerminator(), AC, AllowSpeculation)) {
+ isSafeToExecuteUnconditionally(I, DT, TLI, CurLoop, SafetyInfo, ORE,
+ Preheader->getTerminator(), AC,
+ AllowSpeculation)) {
hoist(I, DT, CurLoop, CFH.getOrCreateHoistedBlock(BB), SafetyInfo,
MSSAU, SE, ORE);
HoistedInstructions.push_back(&I);
|
nikic
requested changes
May 24, 2025
There was a problem hiding this comment.
If you're using stacked pull requests, please link the other PRs from the stack. It's impossible to understand the context otherwise. Based on just the changes in this PR:
- The EarlyCSE and GVN changes do not make sense to me. Those are very much not hoisting transforms, and profitability for hoisting an instruction and removing an instructions through CSE are not at all the same.
- LICM is designed as a target-independent canonicalization pass, which explicitly does not use TTI for cost modelling. There are extensive previous discussions on this topic. From a quick search https://discourse.llvm.org/t/licm-as-canonical-form/59394 has a decent summary.
|
No longer needed, this was previously a prerequisite to #141326 which was changed to a different approach. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
LICM hoists instructions into the preheader, and CSE and GVN
can effectively perform a hoist by replacing an instruction with
one from another basic block, all of which can lead to the same
kinds of pessimizations that isProfitableToHoist() is intended to
prevent. Therefore, call the hook from all these passes.
This will be tested in the subsequent change that adds a previously
hoistable case and verifies that no optimization pass hoists it.