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

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added a comment.

In D99354#2678873 <https://reviews.llvm.org/D99354#2678873>, @fhahn wrote:

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

Yep, It makes 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).

I am sorry for inconvenient. I will follow the review rule.



================
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,
----------------
fhahn wrote:
> `Invariants` here is not really accurate I think. Those are the instructions we need to duplicate outside the loop, right?
Yep, I will change it to `ToToDuplicate`.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:219
+    Instruction *NewInst = Inst->clone();
+    BB.getInstList().insert(BB.end(), NewInst);
+    RemapInstruction(NewInst, VMap,
----------------
fhahn wrote:
> nit: perhaps a bit simpler `NewInst->insertBefore(BB->getTerminator())`
The BB is empty block so we can not use `BB.getTerminator()` here.


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


================
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 =
----------------
fhahn wrote:
> 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?
I agree with you. It is better to provide `isPartiallyInvariant` in  `IVConditionInfo`. If possible, it could be good to create a separate patch for it after pushing this patch.


================
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.
----------------
fhahn wrote:
> nit: `In the partially invariant case, if UnswitchedSuccBB is an exit block, do not ....`
Yep, I will update it.


================
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;
----------------
fhahn wrote:
> no need for `llvm::`, also can we just capture `[&SuccBB]`?
Yep, I will update it.


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


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2692
 
+  struct IVConditionInfo PartialIVInfo;
   for (auto *BB : L.blocks()) {
----------------
fhahn wrote:
> `struct` not needed.
Yep, I will update it.


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


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


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


================
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");
----------------
fhahn wrote:
> this change seems unrelated?
Ah, I ran clang-format. It is result of clang-format.


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


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

https://reviews.llvm.org/D99354