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

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Abhilash,

Please find some stylistic comments from me as well.

Thanks,
Michael



================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:488
+// Return false, otherwise.
+bool LoopUnswitch::isUnreachable(BasicBlock *BB) {
+  auto *Node = DT->getNode(BB)->getIDom();
----------------
Please rename this function, as the name is to generic now, and LLVM already uses term `unreachable` with another meaning. Here the block is rendered unreachable because we unswitched some conditional branches - we should somehow reflect that in the name (I don't have good suggestions though :) ).


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:512
+      return true;
+    }
+  }
----------------
anna wrote:
> Is this clang-formatted? I think single statements within if-conditions dont need braces.
> 
I think clang-format doesn't remove such braces, but yep, we don't need them in this case.


================
Comment at: test/Transforms/LoopUnswitch/elseif-non-exponential-behavior.ll:2-3
+; REQUIRES: asserts
+; RUN: opt -sroa -loop-unswitch -stats -disable-output -info-output-file - < %s | FileCheck --check-prefix=STATS %s 
+; RUN: opt -sroa -loop-unswitch -S < %s | FileCheck %s
+
----------------
Why do we need `-sroa`? Why can't we use its output?

Also, if we do check the actual contents of the function, there is no need in checking debug messages, and thus we can remove first `RUN` invocation and `REQUIRES: asserts`.


================
Comment at: test/Transforms/LoopUnswitch/elseif-non-exponential-behavior.ll:8-12
+; CHECK: for.cond.us:                                      ; preds = %for.inc.us, %entry.split.us
+; CHECK-NEXT:  %pdt.0.us = phi i32 [ 1, %entry.split.us ], [ %pdt.2.us, %for.inc.us ]
+; CHECK-NEXT:  %i.0.us = phi i32 [ 0, %entry.split.us ], [ %inc.us, %for.inc.us ]
+; CHECK-NEXT:  %cmp.us = icmp slt i32 %i.0.us, 100
+; CHECK-NEXT:  br i1 %cmp.us, label %for.body.us, label %for.end.us-lcssa.us
----------------
Some more recommendations about tests:
1) Run `opt -instnamer` on the test so that names like `%7` are replaced with symbolic names. It makes it much easier to edit the test later if necessary (we need to preserve numeration if we don't use symbolic names).
2) Please add `CHECK-LABEL: @b`. That'll help to distinguish function bodies in case someone adds another function into this test.
3) It might be an overkill to check every instruction. It's usually enough to check key instructions in key blocks. E.g.:
```
; CHECK: bb1:
; CHECK-NOT br i1      ; Check that the branch is not conditional any more
; CHECK: br label %succ ; Check that we're unconditionally jumping to correct successor
```
And we don't need to check what's in between label `bb1` and branch from the block. This way it's easier to understand what the test checks, and also the test becomes more robust to random change that can somehow change the bodies of the blocks.


Repository:
  rL LLVM

https://reviews.llvm.org/D26299