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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked 8 inline comments as done.
mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:49
+/// this block in runtime, return this successor. Otherwise, return nullptr.
+static BasicBlock *getOnlyLiveSuccessor(BasicBlock *BB) {
+  Instruction *TI = BB->getTerminator();
----------------
skatkov wrote:
> In terms of compile time improvement I see that in analysis you use this function a lot.
> So it makes sense to cache the result. So you need a set of live pairs <Src, Dest>.
This check is free for branches, for switches it's `O(num_succ)` which is negligible. I think it's better keep it that way than add a cache which can be a source of potential bugs.



================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:160
+      // the processing of child loops.
+      if (TheOnlySucc && LI.getLoopFor(BB) == &L)
+        FoldCandidates.insert(BB);
----------------
skatkov wrote:
> With this check you want to process only blocks from the current loop, not in sub loops. What is the reason for that?
> Simplification?
If we modify some condition in the inner loop, we may end up needing to update its loop info, which we don't want to do. Besides, we expect that all branches in inner loops will have been folded by the moment when we will process the outer loop.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:196
+    // The loop is live is its latch is live.
+    LoopIsLive = IsEdgeLive(L.getLoopLatch(), L.getHeader());
+
----------------
skatkov wrote:
> reames wrote:
> > I think you need a better name for this variable.
> In terms of analysis I think the restriction that there is an only one loop latch is redundant.
It makes things easier, and we canonicalize most loops to this form, so I think we shouldn't bother about multi-latch case. We can support them later if needed.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:345
   // Eliminate unconditional branches by merging blocks into their predecessors.
   Changed |= mergeBlocksIntoPredecessors(L, DT, LI, MSSAU);
 
----------------
skatkov wrote:
> Be careful here in the future, if constantFoldTerminators will eliminate the current loop you should not run merge blocks...
Yes, it will be handled when we support deletion of the current loop (I plan to make it as the last step).


https://reviews.llvm.org/D54021