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

[Anna Thomas via llvm-commits]

anna added a comment.

Hi Abhilash,

I've added some comments mainly related to stylistic changes. Please have a look.

Thanks,
Anna



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


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:627
+
+      if (isUnreachable)
+        continue;
----------------
I think this looks a bit confusing. `isUnreachable` is only set to true once inside the while loop, where we break from it. I think it maybe better to separate the while loop into another function/lambda such as `bool isBranchUnreachable()` and change all the breaks into return false. This way it is easier to read when you have more cases for unreachable branches. 


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:635-636
         // unswitch on it if we desire.
-        Value *LoopCond = FindLIVLoopCondition(BI->getCondition(),
-                                               currentLoop, Changed);
+        Value *LoopCond =
+            FindLIVLoopCondition(BI->getCondition(), currentLoop, Changed);
         if (LoopCond &&
----------------
There's only formatting changes here right? Usually, in diff's it's better to avoid formatting changes not related to your code. I think this case is okay (since it's just a single one). 

If you're using git, there's some script to clang-format *just your changes*.  See `git clang-format`.


================
Comment at: test/Transforms/LoopUnswitch/elseif-non-exponential-behavior.ll:3
+; RUN: opt < %s -O3 -loop-unswitch -stats -disable-output 2>&1 | grep "2 loop-unswitch    - Number of branches unswitched" | count 1
+
+; Function Attrs: nounwind uwtable
----------------
Also, if there are any test cases that the current code will not catch, it will be good to add those. This will help future developers on what would be good cases to work on.


Repository:
  rL LLVM

https://reviews.llvm.org/D26299