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

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

comments inline. Algo looks fine.



================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:198
+
+    if (LoopIsLive) {
+      BlocksInLoopAfterFolding.insert(L.getLoopLatch());
----------------
mkazantsev wrote:
> anna wrote:
> > 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?
> You are right. But this patch is just collecting all required data for follow-up patches https://reviews.llvm.org/D54023 https://reviews.llvm.org/D54025
> 
> My idea was to collect *all* required data in this patch and then add transforms that use it one by one, without adding more analysis. If you think it would be easier to review if the data collection is also added incrementally, I can rework the change.
pls add this with the patch that deletes the loop, instead of in this patch. I'd like to reduce the complexity of this patch.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:288
+    foldTerminators();
+
+    return true;
----------------
mkazantsev wrote:
> anna wrote:
> > We need a bunch of asserts here:
> > 1. assert that L.getHeader() is reachableFromEntry. 
> > 2. assert that LI is correct (mentioned in earlier comment). 
> AFAIK reachibility of header is implied by correctness of LI. Will do.
I checked, but LI.verify() doesn't contain that check for header is reachableFromEntry. We recompute a new LI and then verify original LI and the recomputed one is the same.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:346
+  ConstantTerminatorFoldingImpl BranchFolder(L, LI, DT);
+  Changed |= BranchFolder.run();
+
----------------
You're not running this to a fixed point, so why not just do `Changed = BranchFolder.run()`


================
Comment at: test/Transforms/LoopSimplifyCFG/constant-fold-branch.ll:8
 
+; CHECK-LABEL: In function dead_backedge_test_branch_loop: Give up constant terminator folding in loop header: we don't currently support blocks that are no dead, but will stop being a part of the loop after constant-folding.
+; CHECK-LABEL: In function dead_backedge_test_switch_loop: Give up constant terminator folding in loop header: we don't currently support blocks that are no dead, but will stop being a part of the loop after constant-folding.
----------------
 I understand why you added these, but it's Really a lot of information in the debug output. I think "Give up constant terminator folding in loop header" is enough.


https://reviews.llvm.org/D54021