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

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi,

Please find some comments inline. Also, you still need to add a test for this change.

Michael



================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:596-600
+      /*
+       * Some branches may be rendered unreachable because of previous
+       * unswitching.
+       * Unswitch only those branches that are reachable.
+       */
----------------
Please use the same style for comments as in the rest of the file:
```
// Some branches ...
// ...
// ...
```


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:607
+
+        if (BI && BI->isConditional()) {
+          Value *Cond = BI->getCondition();
----------------
Maybe rewrite it as
```
if (!BI || !BI->isConditional())
  continue;
...
```
?

Then we can also `continue` if `Cond` is not a constant.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:609
+          Value *Cond = BI->getCondition();
+          BasicBlock *Succ = NULL;
+          if (Cond == ConstantInt::getTrue(Cond->getContext())) {
----------------
s/NULL/nullptr/


================
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?
AFAIU, `Succ` is the unreachable successor. Probably, another name would work better here, because indeed, it's confusing.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:630-631
+      if (isUnreachable) {
+        DEBUG(dbgs() << "Unreachable branch: " << *TI << " in Function "
+                     << (*I)->getParent()->getName() << "\n");
+        continue;
----------------
I think this message would be confusing. We either need to provide more details (like why is it unreachable and under which conditions), or drop it completely.


Repository:
  rL LLVM

https://reviews.llvm.org/D26299