[llvm] r288050 - [StructurizeCFG] Refactor NearestCommonDominator.
Justin Lebar via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 28 10:49:59 PST 2016
Author: jlebar
Date: Mon Nov 28 12:49:59 2016
New Revision: 288050
URL: http://llvm.org/viewvc/llvm-project?rev=288050&view=rev
Log:
[StructurizeCFG] Refactor NearestCommonDominator.
Summary:
As far as I can tell, doing our own computations in
NearestCommonDominator is a false optimization -- DomTree will build up
what appears to be exactly this data when it decides it's worthwhile.
Moreover, by building the cache ourselves, we cannot take advantage of
the cache that the domtree might have available.
In addition, I am not convinced of the correctness of the original code.
In particular, setting ResultIndex = 1 on the first addBlock instead of
setting it to 0 is quite fishy. Similarly, it's not clear to me that
setting IndexMap[Node] = 0 for every node as we walk up the tree finding
a common parent is correct. But rather than ponder over these
questions, I'd rather just make the code do the obviously-correct thing.
This patch also changes the NearestCommonDominator API a bit, improving
the names and getting rid of the boolean parameter in addBlock -- see
http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html
Reviewers: arsenm
Subscribers: aemerson, wdng, llvm-commits
Differential Revision: https://reviews.llvm.org/D26998
Modified:
llvm/trunk/lib/Transforms/Scalar/StructurizeCFG.cpp
Modified: llvm/trunk/lib/Transforms/Scalar/StructurizeCFG.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/StructurizeCFG.cpp?rev=288050&r1=288049&r2=288050&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/StructurizeCFG.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/StructurizeCFG.cpp Mon Nov 28 12:49:59 2016
@@ -43,77 +43,58 @@ typedef SmallPtrSet<BasicBlock *, 8> BBS
typedef MapVector<PHINode *, BBValueVector> PhiMap;
typedef MapVector<BasicBlock *, BBVector> BB2BBVecMap;
-typedef DenseMap<DomTreeNode *, unsigned> DTN2UnsignedMap;
typedef DenseMap<BasicBlock *, PhiMap> BBPhiMap;
typedef DenseMap<BasicBlock *, Value *> BBPredicates;
typedef DenseMap<BasicBlock *, BBPredicates> PredMap;
typedef DenseMap<BasicBlock *, BasicBlock*> BB2BBMap;
// The name for newly created blocks.
-
static const char *const FlowBlockName = "Flow";
-/// @brief Find the nearest common dominator for multiple BasicBlocks
+/// Finds the nearest common dominator of a set of BasicBlocks.
///
-/// Helper class for StructurizeCFG
-/// TODO: Maybe move into common code
+/// For every BB you add to the set, you can specify whether we "remember" the
+/// block. When you get the common dominator, you can also ask whether it's one
+/// of the blocks we remembered.
class NearestCommonDominator {
DominatorTree *DT;
+ BasicBlock *Result = nullptr;
+ bool ResultIsRemembered = false;
- DTN2UnsignedMap IndexMap;
-
- BasicBlock *Result;
- unsigned ResultIndex;
- bool ExplicitMentioned;
-
-public:
- /// \brief Start a new query
- NearestCommonDominator(DominatorTree *DomTree) {
- DT = DomTree;
- Result = nullptr;
- }
-
- /// \brief Add BB to the resulting dominator
- void addBlock(BasicBlock *BB, bool Remember = true) {
- DomTreeNode *Node = DT->getNode(BB);
-
+ /// Add BB to the resulting dominator.
+ void addBlock(BasicBlock *BB, bool Remember) {
if (!Result) {
- unsigned Numbering = 0;
- for (;Node;Node = Node->getIDom())
- IndexMap[Node] = ++Numbering;
Result = BB;
- ResultIndex = 1;
- ExplicitMentioned = Remember;
+ ResultIsRemembered = Remember;
return;
}
- for (;Node;Node = Node->getIDom())
- if (IndexMap.count(Node))
- break;
- else
- IndexMap[Node] = 0;
+ BasicBlock *NewResult = DT->findNearestCommonDominator(Result, BB);
+ if (NewResult != Result)
+ ResultIsRemembered = false;
+ if (NewResult == BB)
+ ResultIsRemembered |= Remember;
+ Result = NewResult;
+ }
- assert(Node && "Dominator tree invalid!");
+public:
+ explicit NearestCommonDominator(DominatorTree *DomTree) : DT(DomTree) {}
- unsigned Numbering = IndexMap[Node];
- if (Numbering > ResultIndex) {
- Result = Node->getBlock();
- ResultIndex = Numbering;
- ExplicitMentioned = Remember && (Result == BB);
- } else if (Numbering == ResultIndex) {
- ExplicitMentioned |= Remember;
- }
+ void addBlock(BasicBlock *BB) {
+ addBlock(BB, /* Remember = */ false);
}
- /// \brief Is "Result" one of the BBs added with "Remember" = True?
- bool wasResultExplicitMentioned() {
- return ExplicitMentioned;
+ void addAndRememberBlock(BasicBlock *BB) {
+ addBlock(BB, /* Remember = */ true);
}
- /// \brief Get the query result
- BasicBlock *getResult() {
- return Result;
- }
+ /// Get the nearest common dominator of all the BBs added via addBlock() and
+ /// addAndRememberBlock().
+ BasicBlock *result() { return Result; }
+
+ /// Is the BB returned by getResult() one of the blocks we added to the set
+ /// with addAndRememberBlock()?
+ bool resultIsRememberedBlock() { return ResultIsRemembered; }
};
/// @brief Transforms the control flow graph on one single entry/exit region
@@ -517,7 +498,7 @@ void StructurizeCFG::insertConditions(bo
BBPredicates &Preds = Loops ? LoopPreds[SuccFalse] : Predicates[SuccTrue];
NearestCommonDominator Dominator(DT);
- Dominator.addBlock(Parent, false);
+ Dominator.addBlock(Parent);
Value *ParentValue = nullptr;
for (BBPredicates::iterator PI = Preds.begin(), PE = Preds.end();
@@ -528,14 +509,14 @@ void StructurizeCFG::insertConditions(bo
break;
}
PhiInserter.AddAvailableValue(PI->first, PI->second);
- Dominator.addBlock(PI->first);
+ Dominator.addAndRememberBlock(PI->first);
}
if (ParentValue) {
Term->setCondition(ParentValue);
} else {
- if (!Dominator.wasResultExplicitMentioned())
- PhiInserter.AddAvailableValue(Dominator.getResult(), Default);
+ if (!Dominator.resultIsRememberedBlock())
+ PhiInserter.AddAvailableValue(Dominator.result(), Default);
Term->setCondition(PhiInserter.GetValueInMiddleOfBlock(Parent));
}
@@ -590,15 +571,15 @@ void StructurizeCFG::setPhiValues() {
Updater.AddAvailableValue(To, Undef);
NearestCommonDominator Dominator(DT);
- Dominator.addBlock(To, false);
+ Dominator.addBlock(To);
for (const auto &VI : PI.second) {
Updater.AddAvailableValue(VI.first, VI.second);
- Dominator.addBlock(VI.first);
+ Dominator.addAndRememberBlock(VI.first);
}
- if (!Dominator.wasResultExplicitMentioned())
- Updater.AddAvailableValue(Dominator.getResult(), Undef);
+ if (!Dominator.resultIsRememberedBlock())
+ Updater.AddAvailableValue(Dominator.result(), Undef);
for (BasicBlock *FI : From) {
More information about the llvm-commits
mailing list