[PATCH] D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass.
Sanjoy Das via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat May 6 19:09:53 PDT 2017
sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.
================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:117
+ SmallPtrSet<BasicBlock *, 4> DomSet;
+ auto AppendDomFrontier = [&](DomTreeNode *Node) {
+ assert(DomNodes.empty() && "Must start with no dominator nodes.");
----------------
Minor stylistic thing: this lambda is large enough that I'd prefer pulling it out into a static helper.
I'd also call the parameter something more descriptive than `Node`.
================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:127
+ for (int i = 0; i < (int)DomNodes.size(); ++i)
+ for (DomTreeNode *ChildN : *DomNodes[i])
+ DomNodes.push_back(ChildN);
----------------
This could be `DomNodes.insert(DomNodes.end(), DomNodes[i]->begin(), DomNodes[i]->end());`
================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:135
+ // the DFS numbering.
+ for (DomTreeNode *InnerN : DomNodes)
+ DomSet.insert(InnerN->getBlock());
----------------
I'd be tempted to sort and then binary search instead of building an intermediate set.
================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:142
+ for (BasicBlock *SuccBB : successors(InnerN->getBlock()))
+ if (!DomSet.count(SuccBB))
+ Worklist.insert(SuccBB);
----------------
You're not really computing the dominance frontier here -- the condition for that would be `!DomSet.count(SuccBB) || SuccBB == Node`.
I think the algorithm is correct overall, but this difference from a textbook dominance frontier needs to be documented.
https://reviews.llvm.org/D32740
More information about the llvm-commits
mailing list