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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:192
+    // The loop will not be destroyed if its latch is live.
+    DeleteCurrentLoop = !IsEdgeLive(L.getLoopLatch(), L.getHeader());
+
----------------
I find this name misleading because the current loop can be deleted in 2 ways:
1. latch to header edge is dead
2. edge to header from outside this loop is dead

Second case does not happen now, because you only consider blocks in current loop as candidate for folding. 

However, I think you need to add both cases for completeness here. 






================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:198
+
+    if (LoopIsLive) {
+      BlocksInLoopAfterFolding.insert(L.getLoopLatch());
----------------
mkazantsev wrote:
> anna wrote:
> > 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.
> Sorry for misleading you earlier. This logic works if we *do not* delete the loop. I cannot remove it. It is required to detect the cases we are not supporting yet.
> 
okay, makes sense.


================
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.
----------------
mkazantsev wrote:
> anna wrote:
> >  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.
> Specifying the reason lets us be sure that we exercise all scenarios in this file. Besides, all these messages will be deleted one by one as the optimization is generalized. It's more a temporal thing for current development than something that will stay for long. Let's leave it as is, we're going to delete them anyways.
okay, pls correct typo: "blocks that are not dead". 


https://reviews.llvm.org/D54021