[llvm-branch-commits] [llvm] e113317 - [NFCI][SimplifyCFG] Add basic scaffolding for gradually making the pass DomTree-aware
Roman Lebedev via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Tue Dec 15 13:42:31 PST 2020
Author: Roman Lebedev
Date: 2020-12-16T00:38:00+03:00
New Revision: e1133179587dd895962a2fe4d6eb0cb1e63b5ee2
URL: https://github.com/llvm/llvm-project/commit/e1133179587dd895962a2fe4d6eb0cb1e63b5ee2
DIFF: https://github.com/llvm/llvm-project/commit/e1133179587dd895962a2fe4d6eb0cb1e63b5ee2.diff
LOG: [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.
Added:
Modified:
llvm/include/llvm/Transforms/Utils/Local.h
llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/Transforms/Utils/Local.h b/llvm/include/llvm/Transforms/Utils/Local.h
index fb6f0269a0ac..5a5fb90be1c7 100644
--- a/llvm/include/llvm/Transforms/Utils/Local.h
+++ b/llvm/include/llvm/Transforms/Utils/Local.h
@@ -30,6 +30,7 @@
#include "llvm/IR/Value.h"
#include "llvm/IR/ValueHandle.h"
#include "llvm/Support/Casting.h"
+#include "llvm/Support/CommandLine.h"
#include "llvm/Transforms/Utils/SimplifyCFGOptions.h"
#include <cstdint>
#include <limits>
@@ -186,6 +187,7 @@ bool EliminateDuplicatePHINodes(BasicBlock *BB);
/// It returns true if a modification was made, possibly deleting the basic
/// block that was pointed to. LoopHeaders is an optional input parameter
/// providing the set of loop headers that SimplifyCFG should not eliminate.
+extern cl::opt<bool> RequireAndPreserveDomTree;
bool simplifyCFG(BasicBlock *BB, const TargetTransformInfo &TTI,
const SimplifyCFGOptions &Options = {},
SmallPtrSetImpl<BasicBlock *> *LoopHeaders = nullptr);
diff --git a/llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp b/llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
index c36619211971..deb6494f6b07 100644
--- a/llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
+++ b/llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp
@@ -31,6 +31,7 @@
#include "llvm/IR/CFG.h"
#include "llvm/IR/Constants.h"
#include "llvm/IR/DataLayout.h"
+#include "llvm/IR/Dominators.h"
#include "llvm/IR/Instructions.h"
#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/Module.h"
@@ -191,8 +192,9 @@ static bool iterativelySimplifyCFG(Function &F, const TargetTransformInfo &TTI,
return Changed;
}
-static bool simplifyFunctionCFG(Function &F, const TargetTransformInfo &TTI,
- const SimplifyCFGOptions &Options) {
+static bool simplifyFunctionCFGImpl(Function &F, const TargetTransformInfo &TTI,
+ DominatorTree *DT,
+ const SimplifyCFGOptions &Options) {
bool EverChanged = removeUnreachableBlocks(F);
EverChanged |= mergeEmptyReturnBlocks(F);
EverChanged |= iterativelySimplifyCFG(F, TTI, Options);
@@ -216,6 +218,22 @@ static bool simplifyFunctionCFG(Function &F, const TargetTransformInfo &TTI,
return true;
}
+static bool simplifyFunctionCFG(Function &F, const TargetTransformInfo &TTI,
+ DominatorTree *DT,
+ const SimplifyCFGOptions &Options) {
+ assert((!RequireAndPreserveDomTree ||
+ (DT && DT->verify(DominatorTree::VerificationLevel::Full))) &&
+ "Original domtree is invalid?");
+
+ bool Changed = simplifyFunctionCFGImpl(F, TTI, DT, Options);
+
+ assert((!RequireAndPreserveDomTree ||
+ (DT && DT->verify(DominatorTree::VerificationLevel::Full))) &&
+ "Failed to maintain validity of domtree!");
+
+ return Changed;
+}
+
// Command-line settings override compile-time settings.
static void applyCommandLineOverridesToOptions(SimplifyCFGOptions &Options) {
if (UserBonusInstThreshold.getNumOccurrences())
@@ -245,14 +263,19 @@ PreservedAnalyses SimplifyCFGPass::run(Function &F,
FunctionAnalysisManager &AM) {
auto &TTI = AM.getResult<TargetIRAnalysis>(F);
Options.AC = &AM.getResult<AssumptionAnalysis>(F);
+ DominatorTree *DT = nullptr;
+ if (RequireAndPreserveDomTree)
+ DT = &AM.getResult<DominatorTreeAnalysis>(F);
if (F.hasFnAttribute(Attribute::OptForFuzzing)) {
Options.setSimplifyCondBranch(false).setFoldTwoEntryPHINode(false);
} else {
Options.setSimplifyCondBranch(true).setFoldTwoEntryPHINode(true);
}
- if (!simplifyFunctionCFG(F, TTI, Options))
+ if (!simplifyFunctionCFG(F, TTI, DT, Options))
return PreservedAnalyses::all();
PreservedAnalyses PA;
+ if (RequireAndPreserveDomTree)
+ PA.preserve<DominatorTreeAnalysis>();
PA.preserve<GlobalsAA>();
return PA;
}
@@ -278,6 +301,9 @@ struct CFGSimplifyPass : public FunctionPass {
return false;
Options.AC = &getAnalysis<AssumptionCacheTracker>().getAssumptionCache(F);
+ DominatorTree *DT = nullptr;
+ if (RequireAndPreserveDomTree)
+ DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
if (F.hasFnAttribute(Attribute::OptForFuzzing)) {
Options.setSimplifyCondBranch(false)
.setFoldTwoEntryPHINode(false);
@@ -287,11 +313,15 @@ struct CFGSimplifyPass : public FunctionPass {
}
auto &TTI = getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
- return simplifyFunctionCFG(F, TTI, Options);
+ return simplifyFunctionCFG(F, TTI, DT, Options);
}
void getAnalysisUsage(AnalysisUsage &AU) const override {
AU.addRequired<AssumptionCacheTracker>();
+ if (RequireAndPreserveDomTree)
+ AU.addRequired<DominatorTreeWrapperPass>();
AU.addRequired<TargetTransformInfoWrapperPass>();
+ if (RequireAndPreserveDomTree)
+ AU.addPreserved<DominatorTreeWrapperPass>();
AU.addPreserved<GlobalsAAWrapperPass>();
}
};
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 691f59f0704d..9e9d22c2659c 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -87,6 +87,12 @@ using namespace PatternMatch;
#define DEBUG_TYPE "simplifycfg"
+cl::opt<bool> llvm::RequireAndPreserveDomTree(
+ "simplifycfg-require-and-preserve-domtree", cl::Hidden, cl::ZeroOrMore,
+ cl::init(false),
+ cl::desc("Temorary development switch used to gradually uplift SimplifyCFG "
+ "into preserving DomTree,"));
+
// Chosen as 2 so as to be cheap, but still to have enough power to fold
// a select, so the "clamp" idiom (of a min followed by a max) will be caught.
// To catch this, we need to fold a compare and a select, hence '2' being the
More information about the llvm-branch-commits
mailing list