[PATCH] D32409: [PM/LoopUnswitch] Introduce a new, simpler loop unswitch pass.

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

I focused on the logic to update the dominator and I think it's correct. Some comments below.
Also, recently there have been some changes wrt divergent conditions. Do you plan to integrate them in the new pass?



================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:17
+#include "llvm/Analysis/BlockFrequencyInfoImpl.h"
+#include "llvm/Analysis/BranchProbabilityInfo.h"
+#include "llvm/Analysis/CodeMetrics.h"
----------------
I'm under the impression some of these headers are dead (just copied from the old loop unswitch which had PGO support). Not a big problem, but if you can cleanup them it would be better.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:50
+STATISTIC(NumSwitches, "Number of switches unswitched");
+STATISTIC(NumTrivial, "Number of unswitches that are trivial");
+
----------------
Do we really need `NumTrivial`? From what I see here it's just `NumBranches + NumSwitches`


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:122
+  // exit block.
+  DT.changeImmediateDominator(UnswitchedNode, OldPHNode);
+
----------------
sanjoy wrote:
> Instead of special casing here, why not just
> 
> ```
>   SmallSetVector<BasicBlock *, 4> Worklist;
>   Worklist.insert(UnswitchedNode);
> ```
> 
> and let nature run its course?
+1


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:179-184
+  // FIXME: We should compute this once at the start and update it!
+  SmallVector<BasicBlock *, 16> ExitBlocks;
+  L.getExitBlocks(ExitBlocks);
+  SmallPtrSet<BasicBlock *, 16> ExitBlockSet(ExitBlocks.begin(),
+                                             ExitBlocks.end());
+
----------------
Instead of having `getExitBlocks()` taking a `SmallVector<>` can we have it or a variant of it which takes a `SmallPtrSet` ? 
I could use it in `LCSSA` to compute the dominance chain as well, FWIW.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:527
+    // Skip foldable branches.
+    if (ConstantInt *Cond = dyn_cast<ConstantInt>(BI->getCondition())) {
+      CurrentBB = BI->getSuccessor(Cond->isNullValue() ? 1 : 0);
----------------
Minor: `auto` as the type is obvious.


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:643
+INITIALIZE_PASS_DEPENDENCY(TargetTransformInfoWrapperPass)
+// INITIALIZE_PASS_DEPENDENCY(DivergenceAnalysis)
+INITIALIZE_PASS_END(SimpleLoopUnswitchLegacyPass, "simple-loop-unswitch",
----------------
Commented out code.


https://reviews.llvm.org/D32409