[PATCH] D35528: [Dominators] Teach LoopUnswitch to use the incremental API
Davide Italiano via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 17 21:16:25 PDT 2017
davide added a comment.
I'm a little fried at the moment, so just some drive-by comments. I wonder if we can make the memory management automatic instead of calling naked delete.
================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:938
// insert the new conditional branch.
+ assert(isa<BranchInst>(loopPreheader->getTerminator()));
+ auto *OldBranch = cast<BranchInst>(loopPreheader->getTerminator());
----------------
message in the assertion?
================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:939
+ assert(isa<BranchInst>(loopPreheader->getTerminator()));
+ auto *OldBranch = cast<BranchInst>(loopPreheader->getTerminator());
EmitPreheaderBranchOnCondition(Cond, Val, NewExit, NewPH,
----------------
instead of using isa<> + cast<> can you use `dyn_cast<>` maybe ?
================
Comment at: lib/Transforms/Utils/BreakCriticalEdges.cpp:224-227
if (DT) {
- DomTreeNode *TINode = DT->getNode(TIBB);
-
- // The new block is not the immediate dominator for any other nodes, but
- // TINode is the immediate dominator for the new node.
- //
- if (TINode) { // Don't break unreachable code!
- DomTreeNode *NewBBNode = DT->addNewBlock(NewBB, TIBB);
- DomTreeNode *DestBBNode = nullptr;
-
- // If NewBBDominatesDestBB hasn't been computed yet, do so with DT.
- if (!OtherPreds.empty()) {
- DestBBNode = DT->getNode(DestBB);
- while (!OtherPreds.empty() && NewBBDominatesDestBB) {
- if (DomTreeNode *OPNode = DT->getNode(OtherPreds.back()))
- NewBBDominatesDestBB = DT->dominates(DestBBNode, OPNode);
- OtherPreds.pop_back();
- }
- OtherPreds.clear();
- }
-
- // If NewBBDominatesDestBB, then NewBB dominates DestBB, otherwise it
- // doesn't dominate anything.
- if (NewBBDominatesDestBB) {
- if (!DestBBNode) DestBBNode = DT->getNode(DestBB);
- DT->changeImmediateDominator(DestBBNode, NewBBNode);
- }
- }
+ DT->insertEdge(TIBB, NewBB);
+ DT->insertEdge(NewBB, DestBB);
+ DT->deleteEdge(TIBB, DestBB);
----------------
Can you add a more descriptive comment here?
https://reviews.llvm.org/D35528
More information about the llvm-commits
mailing list