[PATCH] D139109: [LoopUnswitch] Perform loop unswitching on select instructions

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2619
+  if (!isGuaranteedNotToBeUndefOrPoison(Cond, AC, SI, &DT))
+    Cond = new FreezeInst(Cond, Cond->getName() + ".fr", SI);
+  SplitBlockAndInsertIfThen(SI->getCondition(), SI, false,
----------------
Can we change the insertion point to right after the cond? Presumeably it's outside of loop.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2628
+
+  PHINode *Phi = PHINode::Create(SI->getType(), 2, "", SI);
+  Phi->addIncoming(SI->getTrueValue(), ThenBB);
----------------
This is wrong, you can't insert a Phi before SI if there is another instruction before it. Your insertion point should be smth like block.begin().


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2724
   if (DT.dominates(CondBlock, Latch) &&
       (isGuard(&TI) ||
+       (TI.isTerminator() &&
----------------
This condition is growing weird, can we rewrite it to make it explicit that we choose between guard, select, terminator etc? Maybe factor out into a lambda.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2810
 
-    if (CollectGuards)
-      for (auto &I : *BB)
-        if (isGuard(&I)) {
-          auto *Cond =
-              skipTrivialSelect(cast<IntrinsicInst>(&I)->getArgOperand(0));
-          // TODO: Support AND, OR conditions and partial unswitching.
-          if (!isa<Constant>(Cond) && L.isLoopInvariant(Cond))
-            UnswitchCandidates.push_back({&I, {Cond}});
-        }
+    for (auto &I : *BB) {
+      if (auto *SI = dyn_cast<SelectInst>(&I)) {
----------------
Just for record: this will cost us compile time, turning the algorithm from `O(number of blocks)` to `O(number of instructions)` (if guards are off). Keep that in mind if you see compile time regressions.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2816
+        if (!isa<Constant>(Cond) && L.isLoopInvariant(Cond))
+          UnswitchCandidates.push_back({&I, {Cond}});
+      } else if (CollectGuards && isGuard(&I)) {
----------------
I would also filter out such obviously useless cases as:
- Select is not used
- Select is only used outside of the loop (and therefore can be sunk)
maybe smth else, give it some more thought.



================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch.ll:4393
+; Non-trivial loop unswitching of select instruction.
+define i32 @test34(i32 %N, i1 %cond) {
+; CHECK-LABEL: @test34(
----------------
To see what your tests are doing, I propose the following steps:
- Regenerate all checks in this file using update_test_checks.py script;
- Check in your new tests with checks as is (without your change in code)
- Then rebase your patch on top of it, to see what exactly your patch has changed.

Without this, it's hard to tell what's right or wrong here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139109/new/

https://reviews.llvm.org/D139109