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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:48
+/// If \p BB has multiple successors, but only one of them can be reached from
+/// this block in runtime, return this successor. Otherwise, return nullptr.
+static BasicBlock *getOnlyLiveSuccessor(BasicBlock *BB) {
----------------
Please update the comment that you support only branch and switch terminators and for others you just return nullptr.


================
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();
----------------
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>.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:70
+      if (i->getCaseValue() == CI)
+        LiveSucc = i->getCaseSuccessor();
+    return LiveSucc ? LiveSucc : SI->getDefaultDest();
----------------
May be just return this LiveSucc here and after the loop just return the default dest?


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:103
+  // folded.
+  SmallPtrSet<BasicBlock *, 8> FoldCandidates;
+  // The blocks that will still be a part of the current loop after folding.
----------------
According to code how you fill in it, it seems that FoldCandidates contains only blocks from L, not form sub loops...


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:160
+      // the processing of child loops.
+      if (TheOnlySucc && LI.getLoopFor(BB) == &L)
+        FoldCandidates.insert(BB);
----------------
With this check you want to process only blocks from the current loop, not in sub loops. What is the reason for that?
Simplification?


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:196
+    // The loop is live is its latch is live.
+    LoopIsLive = IsEdgeLive(L.getLoopLatch(), L.getHeader());
+
----------------
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.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:279
+
+    // Nothing to constant-fold.
+    if (FoldCandidates.empty())
----------------
It seems that this check should be the first one.
If there is no to fold so no one of the check above will be true.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:345
   // Eliminate unconditional branches by merging blocks into their predecessors.
   Changed |= mergeBlocksIntoPredecessors(L, DT, LI, MSSAU);
 
----------------
Be careful here in the future, if constantFoldTerminators will eliminate the current loop you should not run merge blocks...


https://reviews.llvm.org/D54021