Skip to content

Commit e113317

Browse files
committed
[NFCI][SimplifyCFG] Add basic scaffolding for gradually making the pass DomTree-aware
Two observations: 1. Unavailability of DomTree makes it impossible to make `FoldBranchToCommonDest()` transform in certain cases, where the successor is dominated by predecessor, because we then don't have PHI's, and can't recreate them, well, without handrolling 'is dominated by' check, which doesn't really look like a great solution to me. 2. Avoiding invalidating DomTree in SimplifyCFG will decrease the number of `Dominator Tree Construction` by 5 (from 28 now, i.e. -18%) in `-O3` old-pm pipeline (as per `llvm/test/Other/opt-O3-pipeline.ll`) This might or might not be beneficial for compile time. So the plan is to make SimplifyCFG preserve DomTree, and then eventually make DomTree fully required and preserved by the pass. Now, SimplifyCFG is ~7KLOC. I don't think it will be nice to do all this uplifting in a single mega-commit, nor would it be possible to review it in any meaningful way. But, i believe, it should be possible to do this in smaller steps, introducing the new behavior, in an optional way, off-by-default, opt-in option, and gradually fixing transforms one-by-one and adding the flag to appropriate test coverage. Then, eventually, the default should be flipped, and eventually^2 the flag removed. And that is what is happening here - when the new off-by-default option is specified, DomTree is required and is claimed to be preserved, and SimplifyCFG-internal assertions verify that the DomTree is still OK.
1 parent a7deedc commit e113317

File tree

3 files changed

+42
-4
lines changed

3 files changed

+42
-4
lines changed

llvm/include/llvm/Transforms/Utils/Local.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
#include "llvm/IR/Value.h"
3131
#include "llvm/IR/ValueHandle.h"
3232
#include "llvm/Support/Casting.h"
33+
#include "llvm/Support/CommandLine.h"
3334
#include "llvm/Transforms/Utils/SimplifyCFGOptions.h"
3435
#include <cstdint>
3536
#include <limits>
@@ -186,6 +187,7 @@ bool EliminateDuplicatePHINodes(BasicBlock *BB);
186187
/// It returns true if a modification was made, possibly deleting the basic
187188
/// block that was pointed to. LoopHeaders is an optional input parameter
188189
/// providing the set of loop headers that SimplifyCFG should not eliminate.
190+
extern cl::opt<bool> RequireAndPreserveDomTree;
189191
bool simplifyCFG(BasicBlock *BB, const TargetTransformInfo &TTI,
190192
const SimplifyCFGOptions &Options = {},
191193
SmallPtrSetImpl<BasicBlock *> *LoopHeaders = nullptr);

llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
#include "llvm/IR/CFG.h"
3232
#include "llvm/IR/Constants.h"
3333
#include "llvm/IR/DataLayout.h"
34+
#include "llvm/IR/Dominators.h"
3435
#include "llvm/IR/Instructions.h"
3536
#include "llvm/IR/IntrinsicInst.h"
3637
#include "llvm/IR/Module.h"
@@ -191,8 +192,9 @@ static bool iterativelySimplifyCFG(Function &F, const TargetTransformInfo &TTI,
191192
return Changed;
192193
}
193194

194-
static bool simplifyFunctionCFG(Function &F, const TargetTransformInfo &TTI,
195-
const SimplifyCFGOptions &Options) {
195+
static bool simplifyFunctionCFGImpl(Function &F, const TargetTransformInfo &TTI,
196+
DominatorTree *DT,
197+
const SimplifyCFGOptions &Options) {
196198
bool EverChanged = removeUnreachableBlocks(F);
197199
EverChanged |= mergeEmptyReturnBlocks(F);
198200
EverChanged |= iterativelySimplifyCFG(F, TTI, Options);
@@ -216,6 +218,22 @@ static bool simplifyFunctionCFG(Function &F, const TargetTransformInfo &TTI,
216218
return true;
217219
}
218220

221+
static bool simplifyFunctionCFG(Function &F, const TargetTransformInfo &TTI,
222+
DominatorTree *DT,
223+
const SimplifyCFGOptions &Options) {
224+
assert((!RequireAndPreserveDomTree ||
225+
(DT && DT->verify(DominatorTree::VerificationLevel::Full))) &&
226+
"Original domtree is invalid?");
227+
228+
bool Changed = simplifyFunctionCFGImpl(F, TTI, DT, Options);
229+
230+
assert((!RequireAndPreserveDomTree ||
231+
(DT && DT->verify(DominatorTree::VerificationLevel::Full))) &&
232+
"Failed to maintain validity of domtree!");
233+
234+
return Changed;
235+
}
236+
219237
// Command-line settings override compile-time settings.
220238
static void applyCommandLineOverridesToOptions(SimplifyCFGOptions &Options) {
221239
if (UserBonusInstThreshold.getNumOccurrences())
@@ -245,14 +263,19 @@ PreservedAnalyses SimplifyCFGPass::run(Function &F,
245263
FunctionAnalysisManager &AM) {
246264
auto &TTI = AM.getResult<TargetIRAnalysis>(F);
247265
Options.AC = &AM.getResult<AssumptionAnalysis>(F);
266+
DominatorTree *DT = nullptr;
267+
if (RequireAndPreserveDomTree)
268+
DT = &AM.getResult<DominatorTreeAnalysis>(F);
248269
if (F.hasFnAttribute(Attribute::OptForFuzzing)) {
249270
Options.setSimplifyCondBranch(false).setFoldTwoEntryPHINode(false);
250271
} else {
251272
Options.setSimplifyCondBranch(true).setFoldTwoEntryPHINode(true);
252273
}
253-
if (!simplifyFunctionCFG(F, TTI, Options))
274+
if (!simplifyFunctionCFG(F, TTI, DT, Options))
254275
return PreservedAnalyses::all();
255276
PreservedAnalyses PA;
277+
if (RequireAndPreserveDomTree)
278+
PA.preserve<DominatorTreeAnalysis>();
256279
PA.preserve<GlobalsAA>();
257280
return PA;
258281
}
@@ -278,6 +301,9 @@ struct CFGSimplifyPass : public FunctionPass {
278301
return false;
279302

280303
Options.AC = &getAnalysis<AssumptionCacheTracker>().getAssumptionCache(F);
304+
DominatorTree *DT = nullptr;
305+
if (RequireAndPreserveDomTree)
306+
DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
281307
if (F.hasFnAttribute(Attribute::OptForFuzzing)) {
282308
Options.setSimplifyCondBranch(false)
283309
.setFoldTwoEntryPHINode(false);
@@ -287,11 +313,15 @@ struct CFGSimplifyPass : public FunctionPass {
287313
}
288314

289315
auto &TTI = getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
290-
return simplifyFunctionCFG(F, TTI, Options);
316+
return simplifyFunctionCFG(F, TTI, DT, Options);
291317
}
292318
void getAnalysisUsage(AnalysisUsage &AU) const override {
293319
AU.addRequired<AssumptionCacheTracker>();
320+
if (RequireAndPreserveDomTree)
321+
AU.addRequired<DominatorTreeWrapperPass>();
294322
AU.addRequired<TargetTransformInfoWrapperPass>();
323+
if (RequireAndPreserveDomTree)
324+
AU.addPreserved<DominatorTreeWrapperPass>();
295325
AU.addPreserved<GlobalsAAWrapperPass>();
296326
}
297327
};

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,12 @@ using namespace PatternMatch;
8787

8888
#define DEBUG_TYPE "simplifycfg"
8989

90+
cl::opt<bool> llvm::RequireAndPreserveDomTree(
91+
"simplifycfg-require-and-preserve-domtree", cl::Hidden, cl::ZeroOrMore,
92+
cl::init(false),
93+
cl::desc("Temorary development switch used to gradually uplift SimplifyCFG "
94+
"into preserving DomTree,"));
95+
9096
// Chosen as 2 so as to be cheap, but still to have enough power to fold
9197
// a select, so the "clamp" idiom (of a min followed by a max) will be caught.
9298
// To catch this, we need to fold a compare and a select, hence '2' being the

0 commit comments

Comments
 (0)