[PATCH] D99354: [SimpleLoopUnswitch] Port partially invariant unswitch from LoopUnswitch to SimpleLoopUnswitch

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Does this patch give the expected speedup on `omnetpp`?

In D99354#2676137 <https://reviews.llvm.org/D99354#2676137>, @jaykang10 wrote:

> @fhahn Can you review this change when you have time please?

please keep the 'common courtesy ping rate' of 1 week in mind (https://llvm.org/docs/CodeReview.html#code-reviews-speed-and-reciprocity), also considering that doing a proper review can take a substantial amount of time for a reviewer and there can be a number of reasons for a delayed response (like holidays, other urgent work).



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:212
+static void buildPartialInvariantUnswitchConditionalBranch(
+    BasicBlock &BB, ArrayRef<Value *> Invariants, bool Direction,
+    BasicBlock &UnswitchedSucc, BasicBlock &NormalSucc, Loop &L,
----------------
`Invariants` here is not really accurate I think. Those are the instructions we need to duplicate outside the loop, right?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:219
+    Instruction *NewInst = Inst->clone();
+    BB.getInstList().insert(BB.end(), NewInst);
+    RemapInstruction(NewInst, VMap,
----------------
nit: perhaps a bit simpler `NewInst->insertBefore(BB->getTerminator())`


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2015
+    SmallVectorImpl<BasicBlock *> &ExitBlocks,
+    struct IVConditionInfo &PartialIVInfo, DominatorTree &DT, LoopInfo &LI,
+    AssumptionCache &AC,
----------------
`struct` not needed.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2028
          "Can only unswitch switches and conditional branch!");
-  bool FullUnswitch = SI || BI->getCondition() == Invariants[0];
+  bool PartiallyInvariant = !PartialIVInfo.InstToDuplicate.empty();
+  bool FullUnswitch =
----------------
I think we should probably update `IVConditionInfo` to provide a better way to check if there are partially invariant conditions, e.g. an `isPartiallyInvariant` accessor. WDYT?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2149
   for (auto *SuccBB : UnswitchedSuccBBs) {
-    VMaps.emplace_back(new ValueToValueMapTy());
-    ClonedPHs[SuccBB] = buildClonedLoopBlocks(
-        L, LoopPH, SplitBB, ExitBlocks, ParentBB, SuccBB, RetainedSuccBB,
-        DominatingSucc, *VMaps.back(), DTUpdates, AC, DT, LI, MSSAU);
+    // In Partially invariant case, if UnswithcedSuccBB is exit block, do not
+    // clone loop and assigned the UnswitchedSuccBB to ClonedPHs.
----------------
nit: `In the partially invariant case, if UnswitchedSuccBB is an exit block, do not ....`


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2151
+    // clone loop and assigned the UnswitchedSuccBB to ClonedPHs.
+    if (PartiallyInvariant && llvm::any_of(ExitBlocks, [&](BasicBlock *ExitBB) {
+          return ExitBB == SuccBB;
----------------
no need for `llvm::`, also can we just capture `[&SuccBB]`?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2375
         Instruction *UserI = dyn_cast<Instruction>(U.getUser());
-        if (!UserI)
+        if (!UserI || PartiallyInvariant)
           continue;
----------------
We don't need to execute this loop at all for the `PartiallyInvariant` case, right?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2692
 
+  struct IVConditionInfo PartialIVInfo;
   for (auto *BB : L.blocks()) {
----------------
`struct` not needed.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2745
+  if (MSSAU &&
+      !llvm::any_of(UnswitchCandidates, [&](auto &TerminatorAndInvariants) {
+        return TerminatorAndInvariants.first == L.getHeader()->getTerminator();
----------------
nit: no `llvm::` should be needed.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2749
+    MemorySSA *MSSA = MSSAU->getMemorySSA();
+    if (auto Info = llvm::hasPartialIVCondition(L, MSSAThreshold, *MSSA, AA)) {
+      LLVM_DEBUG(
----------------
nit: no `llvm::` should be needed


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2878
+        } else if (!PartialIVInfo.InstToDuplicate.empty()) {
+          if (PartialIVInfo.KnownValue->isOneValue() &&
+              SuccBB == BI.getSuccessor(1))
----------------
Do we need to check here that we only do this for blocks where we partially unswitch on their conditions?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2957
 
-  LLVM_DEBUG(dbgs() << "  Unswitching non-trivial (cost = "
-                    << BestUnswitchCost << ") terminator: " << *BestUnswitchTI
-                    << "\n");
+  LLVM_DEBUG(dbgs() << "  Unswitching non-trivial (cost = " << BestUnswitchCost
+                    << ") terminator: " << *BestUnswitchTI << "\n");
----------------
this change seems unrelated?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3161
+    if (CurrentLoopValid) {
+      if (!PartiallyInvariant)
+        LPM.addLoop(*L);
----------------
can you add a comment here to explain the check?


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

https://reviews.llvm.org/D99354