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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

One final round of comments, after that I think this LG.

Could you also add a test for the threshold (like `llvm/test/Transforms/LoopUnswitch/partial-unswitch-mssa-threshold.ll`) and the interesting MemorySSA update cases from `llvm/test/Transforms/LoopUnswitch/partial-unswitch-update-memoryssa.ll`?



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:210
 
+/// Copy a set of loop invariant values, and conditionally branch on them.
+static void buildPartialInvariantUnswitchConditionalBranch(
----------------
nit:

`Copy a set of loop invariant values \p ToDuplicate and insert them at the end of \p BB....`?


 ... and conditional branch on the copied condition.` We only branch on a single value.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2053
+            PartiallyInvariant) &&
            "Only `or`, `and`, an `select` instructions can combine "
            "invariants being unswitched.");
----------------
message in assert needs updating?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2735
 
-    UnswitchCandidates.push_back({BI, std::move(Invariants)});
+  if (MSSAU && !any_of(UnswitchCandidates, [&](auto &TerminatorAndInvariants) {
+        return TerminatorAndInvariants.first == L.getHeader()->getTerminator();
----------------
enough to capture `[this]`?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2744
+      PartialIVInfo = *Info;
+      TinyPtrVector<Value *> ValsToDuplicate;
+      for (auto *Inst : Info->InstToDuplicate)
----------------
Why `TinyPtrVector` here? In most case we will have at least 2 instructions here anyways? Probably better to use a `SmallVector`?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2747
+        ValsToDuplicate.push_back(Inst);
+      UnswitchCandidates.push_back(
+          {L.getHeader()->getTerminator(), std::move(ValsToDuplicate)});
----------------
`emplace_back`?


================
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 =
----------------
jaykang10 wrote:
> 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.
Sounds good, looking forward to a follow-up!


================
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");
----------------
jaykang10 wrote:
> fhahn wrote:
> > this change seems unrelated?
> Ah, I ran clang-format. It is result of clang-format.
ok, can you undo it? As it is not related to the change.


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

https://reviews.llvm.org/D99354