[PATCH] D26299: Improving the efficiency of the Loop Unswitching pass

[Abhilash Bhandari via llvm-commits]

Abhilash marked an inline comment as done.
Abhilash added a comment.

In https://reviews.llvm.org/D26299#601308, @mzolotukhin wrote:

> Hi Abhilash,
>
> The change looks good to me now, thank you! Regarding phis - I would probably just check how many operands the phi-instruction has, without explicit specifying the operands (if it doesn't make the regex much easier, then it's ok to keep it as you have it now I think). Thanks for your patience in going through so many iterations of review! :)
>
> Michael


Thank you Michael. It's a learning phase for me.

---

Abhilash



================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:609
+          Value *Cond = BI->getCondition();
+          BasicBlock *Succ = NULL;
+          if (Cond == ConstantInt::getTrue(Cond->getContext())) {
----------------
mzolotukhin wrote:
> s/NULL/nullptr/
The nullptr initialization is no longer required and hence has been removed.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:611
+          if (Cond == ConstantInt::getTrue(Cond->getContext())) {
+            Succ = BI->getSuccessor(1);
+          } else if (Cond == ConstantInt::getFalse(Cond->getContext())) {
----------------
weimingz wrote:
> Does the True destination and False destination get swapped?
No. The code just identifies the unreachable successors. 
After unswitching, some of the branch conditions may just be constants (True/false). If the condition is true, then the else (false) path is unreachable and otherwise.
All the branches that are unreachable (because of previous unswitching) should not be considered for further unswitching. The code filters out such unreachable conditions.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:620-621
+
+        BasicBlockEdge UnreachableEdge(BInst->getParent(), UnreachableSucc);
+        if (DT->dominates(UnreachableEdge, *I)) {
+          isUnreachable = true;
----------------
anna wrote:
> Isn't it enough to check if the basic block `UnreachableSucc` dominates `I->getParent()`? 
The change has been incorporated.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:510
+    BasicBlockEdge UnreachableEdge(BInst->getParent(), UnreachableSucc);
+    if (DT->dominates(UnreachableEdge, BB)) {
+      return true;
----------------
anna wrote:
> Prior diff question:
> Isn't it enough to check if the basic block UnreachableSucc dominates I->getParent()?
> i.e. `DT->dominates(UnreachableSucc, BB). 
> Is there something else subtle here, that I'm missing?
> IIUC, the algorithm keeps traversing backwards to an immediate dominator of the current `DomBB`, and checks if it's an unreachable block. 
> 
I have rectified this in the latest revision.


Repository:
  rL LLVM

https://reviews.llvm.org/D26299