[PATCH] D54021: [LoopSimplifyCFG] Teach LoopSimplifyCFG to constant-fold branches and switches

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

Couple of comments inline. Pls add test cases.



================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:64
+    auto *CI = dyn_cast<ConstantInt>(SI->getCondition());
+    if (!SI)
+      return nullptr;
----------------
dmgreen wrote:
> Should be CI?
yup, looks like a typo.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:136
+  /// Fill all information about status of blocks and exits of the current loop
+  /// after constant folding has been done.
+  void analyze() {
----------------
`if constant folding will be done`. At this point CF is not done.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:159
+      // are skipped because if they can be folded, they should be folded during
+      // the processing of child loops.
+      if (TheOnlySucc && LI.getLoopFor(BB) == &L)
----------------
Pls add test cases where:
1. Block's only incoming edge if constant folded will make the block dead - this transform shouldn't kick in.
2. Block has multiple incoming edges - even if all except one is constant folded, the block is still live. 


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:198
+
+    if (LoopIsLive) {
+      BlocksInLoopAfterFolding.insert(L.getLoopLatch());
----------------
reames wrote:
> Not clear to me why you need this bit of code in this patch.  
If `LoopIsLive` is false, we should just return at this point now, right? For this patch, I believe this chunk of code is not needed. Am I missing something?


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:261
+    assert(L.getLoopLatch() && "Should be single latch!");
+
+    // TODO: Support deletion of dead loop blocks.
----------------
Pls add test cases for each of the scenarios described below. The constantfold xform should kick in with this patch.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:288
+    foldTerminators();
+
+    return true;
----------------
We need a bunch of asserts here:
1. assert that L.getHeader() is reachableFromEntry. 
2. assert that LI is correct (mentioned in earlier comment). 


https://reviews.llvm.org/D54021