[PATCH] D19257: [SimplifyCFG] Extend TryToSimplifyUncondBranchFromEmptyBlock for empty block including lifetime intrinsics
Hans Wennborg via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 22 09:44:34 PDT 2016
hans added a comment.
I've put some comments in the code, but my main concern is that this patch needs a lot of test cases.
================
Comment at: include/llvm/IR/BasicBlock.h:159
@@ +158,3 @@
+ /// instruction
+ /// coupled with the following lifetime intrinsic.
+ Instruction *getFirstNonPHIOrDbgOrLifetimeOrBitCast();
----------------
nit: The formatting of this comment doesn't look right.
================
Comment at: lib/IR/BasicBlock.cpp:216
@@ +215,3 @@
+ if (auto *II = dyn_cast<IntrinsicInst>(&I))
+ if (II->getIntrinsicID() == Intrinsic::lifetime_end)
+ continue;
----------------
The getFirstNonPHIOrDbgOrLifetime() function also considers Intrinsic::lifetime_start. Should this function do that too?
================
Comment at: lib/Transforms/Utils/Local.cpp:799
@@ -798,1 +798,3 @@
+/// hasLifetimeEnd - Return true if BB has lifetime.end intrinsic.
+///
----------------
No need to repeat the name of the function in the comment. This applies to the other functions as well.
================
Comment at: lib/Transforms/Utils/Local.cpp:803
@@ +802,3 @@
+ BasicBlock::iterator I = BB->begin();
+ while (!I->isTerminator()) {
+ if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(I))
----------------
How about writing this with a range-based for-loop instead?
================
Comment at: lib/Transforms/Utils/Local.cpp:816
@@ +815,3 @@
+ BasicBlock::iterator I = BB->begin();
+ return isa<PHINode>(I) ? true : false;
+}
----------------
No need for the the ?: operator here.
In fact, how about just:
return isa<PHINode>(BB->begin());
And that means maybe we don't need a separate function for this? For example, on line 977 we already do the same kind of check with "if (isa<PHINode>(Succ->begin())) {"
================
Comment at: lib/Transforms/Utils/Local.cpp:825
@@ +824,3 @@
+ BasicBlock::iterator I = BB->begin();
+ BasicBlock::iterator NI = BB->begin();
+
----------------
No need to declare these before they're used.
================
Comment at: lib/Transforms/Utils/Local.cpp:830
@@ +829,3 @@
+ // we cannot hoist lifetime.end intrinsics because it would change semantics.
+ for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI) {
+ BasicBlock *Pred = *PI;
----------------
You could use a range-based for-loop over predecessors(BB) instead
================
Comment at: lib/Transforms/Utils/Local.cpp:834
@@ +833,3 @@
+ if (auto *BI = dyn_cast<BranchInst>(PTI)) {
+ if (BI->isUnconditional())
+ continue;
----------------
Would it be sufficient to just check "BI->getSingleSuccessor()"?
================
Comment at: lib/Transforms/Utils/Local.cpp:839
@@ +838,3 @@
+ } else
+ return false;
+ }
----------------
Some of these "else" and "return false" lines seem unnecessary.. how about something like:
for (each predecessor P) {
if (not P has single successor)
return false;
}
================
Comment at: lib/Transforms/Utils/Local.cpp:846
@@ +845,3 @@
+ if (isa<BitCastInst>(I)) {
+ for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI) {
+ BasicBlock *Pred = *PI;
----------------
Again, range-based for-loop would be nice.
================
Comment at: lib/Transforms/Utils/Local.cpp:849
@@ +848,3 @@
+ pred_iterator NPI = PI;
+ if (++NPI == E) {
+ Pred->getInstList().splice(Pred->getTerminator()->getIterator(),
----------------
I'm not sure this extra logic to splice instead of clone the instruction into the predecessor is worth it.
================
Comment at: lib/Transforms/Utils/Local.cpp:855
@@ +854,3 @@
+ }
+ I = BB->begin();
+ NI = BB->begin();
----------------
I suppose this is because we might have moved I into a predecessor? This makes the loop hard to follow. If you used cloned the instruction into the predecessor, there would be no need for this, right?
================
Comment at: lib/Transforms/Utils/Local.cpp:873
@@ +872,3 @@
+ if ((PEI != Pred->begin()) && isa<BitCastInst>(--PEI))
+ NPEI->setOperand(1, &*(PEI));
+ }
----------------
This all looks very complicated.
================
Comment at: lib/Transforms/Utils/Local.cpp:881
@@ +880,3 @@
+ } else
+ return false;
+ }
----------------
Didn't we check earlier the contents of the BB so we know it can be hoisted? In that case there shouldn't be a need for all this checking and "return false" statements. Can you just add asserts that the instructions are what we expect, and then clone them to the predecessors?
================
Comment at: lib/Transforms/Utils/Local.cpp:905
@@ +904,3 @@
+
+ for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI)
+ if (hasPHI(*PI))
----------------
You could use a range-based for-loop over predecessors(BB) instead, and successors(BB) in the loop below.
================
Comment at: lib/Transforms/Utils/Local.cpp:917
@@ +916,3 @@
+
+ // Copy over any phi, debug or lifetime instruction.
+ BB->getTerminator()->eraseFromParent();
----------------
Didn't we check above that BB has no phi instructions?
http://reviews.llvm.org/D19257
More information about the llvm-commits
mailing list