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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


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


================
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)
----------------
anna wrote:
> 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. 
Already in `test/Transforms/LoopSimplifyCFG/constant-fold-branch.ll`, it has a variety of such cases with inner/exit blocks and loops becoming dead.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:182
+    // Now, all exit blocks that are not marked as live are dead.
+    for (auto *ExitBlock : ExitBlocks)
+      if (!LiveExitBlocks.count(ExitBlock))
----------------
reames wrote:
> Why bother to keep around both sets when given an exit block you can answer liveness questions with only one copy?  (Same question on other parallel dead/live structures BTW)
As we support more cases in follow-up patches, sets of live and dead blocks have different purposes and are all used. See:
https://reviews.llvm.org/D54023
https://reviews.llvm.org/D54025

This patch just collects all required information and enables the simplest case (that doesn't need all of it). However, as we add more logic, these sets are used more actively.

All this logic is only split into two patches to make it easier to review.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:198
+
+    if (LoopIsLive) {
+      BlocksInLoopAfterFolding.insert(L.getLoopLatch());
----------------
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.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:248
+      for (auto *DeadSucc : DeadSuccessors)
+        DTU.deleteEdge(BB, DeadSucc);
+
----------------
anna wrote:
> reames wrote:
> > Where's the code for LI updating?
> From the summary of this patch, it looks like we don't need LI update in this patch:
> ```
> Only the simplest case supported in this patch: after the folding, no block
> should become dead or stop being part of the loop. Support for more
> sophisticated cases will go separately in follow-up patches.
> ```
> 
> If that's the case, I'd really like an assert to make sure that the LI is still correct after this transform is completed. 
> 
> I'll review the code to see if what's stated here is actually true (for example, we have a number of prechecks before performing the actual transform). See `run` function below.
> 
> 
No `LI` update is needed because no block gets deleted or leaves the loop. This patch only handles the situation when it is so. You can take a look on follow-up dependent patches, they have LI updates.

I will add the assert that `LI` is still correct after the transform.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:261
+    assert(L.getLoopLatch() && "Should be single latch!");
+
+    // TODO: Support deletion of dead loop blocks.
----------------
anna wrote:
> Pls add test cases for each of the scenarios described below. The constantfold xform should kick in with this patch.
All in `test/Transforms/LoopSimplifyCFG/constant-fold-branch.ll`. Currently they are all negative tests. Patches to this pass enable groups of them.


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:288
+    foldTerminators();
+
+    return true;
----------------
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.


https://reviews.llvm.org/D54021