[PATCH] Fix dominator descendants for unreachable blocks.
Diego Novillo
dnovillo at google.com
Mon Dec 2 06:37:42 PST 2013
I can't find a way to re-open a diff in phabricator. I'm attaching
the final version I've just committed.
Diego.
On Mon, Dec 2, 2013 at 9:14 AM, Diego Novillo <dnovillo at google.com> wrote:
>
> Thanks for the review. Committed r196099.
>
>
> ================
> Comment at: include/llvm/Analysis/Dominators.h:356
> @@ -355,3 +355,3 @@
>
> - while (!WL.empty()) {
> + while (!WL.empty() && RN) {
> const DomTreeNodeBase<NodeT> *N = WL.pop_back_val();
> ----------------
> Chandler Carruth wrote:
>> No need to test RN in the loop condition, its invariant. Also, no need to build the worklist when RN is null.
>>
>> I would change this to something like:
>>
>> Result.clear();
>> const DomTreeNodeBase<NodeT> *RN = getNode(R);
>> if (!RN)
>> return; // The descendants are the empty set for unreachable code.
>> WL.push_back(RN);
>>
> Testing RN in the loop is not a problem, assuming code motion is doing what it should. I agree that it's best not to even try building the work list in that case.
>
> ================
> Comment at: unittests/IR/DominatorTreeTest.cpp:168-173
> @@ +167,8 @@
> +
> + DominatedBBs.clear();
> + PostDominatedBBs.clear();
> + DT->getDescendants(BB3, DominatedBBs);
> + DT->getDescendants(BB3, PostDominatedBBs);
> + EXPECT_EQ(DominatedBBs.size(), 0UL);
> + EXPECT_EQ(PostDominatedBBs.size(), 0UL);
> +
> ----------------
> Chandler Carruth wrote:
>> Comment that you're testing the behavior for unreachable basic blocks?
> Done.
>
>
> http://llvm-reviews.chandlerc.com/D2288
-------------- next part --------------
diff --git a/include/llvm/Analysis/Dominators.h b/include/llvm/Analysis/Dominators.h
index e35e101..896664c 100644
--- a/include/llvm/Analysis/Dominators.h
+++ b/include/llvm/Analysis/Dominators.h
@@ -348,10 +348,12 @@ public:
/// Get all nodes dominated by R, including R itself.
void getDescendants(NodeT *R, SmallVectorImpl<NodeT *> &Result) const {
+ Result.clear();
const DomTreeNodeBase<NodeT> *RN = getNode(R);
+ if (RN == NULL)
+ return; // If R is unreachable, it will not be present in the DOM tree.
SmallVector<const DomTreeNodeBase<NodeT> *, 8> WL;
WL.push_back(RN);
- Result.clear();
while (!WL.empty()) {
const DomTreeNodeBase<NodeT> *N = WL.pop_back_val();
diff --git a/unittests/IR/DominatorTreeTest.cpp b/unittests/IR/DominatorTreeTest.cpp
index 4e5af93..387d732 100644
--- a/unittests/IR/DominatorTreeTest.cpp
+++ b/unittests/IR/DominatorTreeTest.cpp
@@ -8,6 +8,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/Analysis/Dominators.h"
+#include "llvm/Analysis/PostDominators.h"
#include "llvm/Assembly/Parser.h"
#include "llvm/IR/Instructions.h"
#include "llvm/IR/LLVMContext.h"
@@ -26,6 +27,7 @@ namespace llvm {
static char ID;
virtual bool runOnFunction(Function &F) {
DominatorTree *DT = &getAnalysis<DominatorTree>();
+ PostDominatorTree *PDT = &getAnalysis<PostDominatorTree>();
Function::iterator FI = F.begin();
BasicBlock *BB0 = FI++;
@@ -148,10 +150,34 @@ namespace llvm {
EXPECT_TRUE(DT->dominates(Y6, BB3));
+ // Post dominance.
+ EXPECT_TRUE(PDT->dominates(BB0, BB0));
+ EXPECT_FALSE(PDT->dominates(BB1, BB0));
+ EXPECT_FALSE(PDT->dominates(BB2, BB0));
+ EXPECT_FALSE(PDT->dominates(BB3, BB0));
+ EXPECT_TRUE(PDT->dominates(BB4, BB1));
+
+ // Dominance descendants.
+ SmallVector<BasicBlock *, 8> DominatedBBs, PostDominatedBBs;
+
+ DT->getDescendants(BB0, DominatedBBs);
+ PDT->getDescendants(BB0, PostDominatedBBs);
+ EXPECT_EQ(DominatedBBs.size(), 4UL);
+ EXPECT_EQ(PostDominatedBBs.size(), 1UL);
+
+ // BB3 is unreachable. It should have no dominators nor postdominators.
+ DominatedBBs.clear();
+ PostDominatedBBs.clear();
+ DT->getDescendants(BB3, DominatedBBs);
+ DT->getDescendants(BB3, PostDominatedBBs);
+ EXPECT_EQ(DominatedBBs.size(), 0UL);
+ EXPECT_EQ(PostDominatedBBs.size(), 0UL);
+
return false;
}
virtual void getAnalysisUsage(AnalysisUsage &AU) const {
AU.addRequired<DominatorTree>();
+ AU.addRequired<PostDominatorTree>();
}
DPass() : FunctionPass(ID) {
initializeDPassPass(*PassRegistry::getPassRegistry());
@@ -201,4 +227,5 @@ namespace llvm {
INITIALIZE_PASS_BEGIN(DPass, "dpass", "dpass", false, false)
INITIALIZE_PASS_DEPENDENCY(DominatorTree)
+INITIALIZE_PASS_DEPENDENCY(PostDominatorTree)
INITIALIZE_PASS_END(DPass, "dpass", "dpass", false, false)
More information about the llvm-commits
mailing list