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

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added a comment.

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

> 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`?

Yep, let me try to add the tests.



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:210
 
+/// Copy a set of loop invariant values, and conditionally branch on them.
+static void buildPartialInvariantUnswitchConditionalBranch(
----------------
fhahn wrote:
> 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.
Yep, I will update it.


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


================
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();
----------------
fhahn wrote:
> enough to capture `[this]`?
This code is inside static function rather than class's member function so we can not use `[this]`.  I will update it with `[&L]`.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2744
+      PartialIVInfo = *Info;
+      TinyPtrVector<Value *> ValsToDuplicate;
+      for (auto *Inst : Info->InstToDuplicate)
----------------
fhahn wrote:
> Why `TinyPtrVector` here? In most case we will have at least 2 instructions here anyways? Probably better to use a `SmallVector`?
The SimpleUnswitchPass uses `TinyPtrVector` for `UnswitchCandidates`. In order to follow the interface, `TinyPtrVector` is being used here.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2747
+        ValsToDuplicate.push_back(Inst);
+      UnswitchCandidates.push_back(
+          {L.getHeader()->getTerminator(), std::move(ValsToDuplicate)});
----------------
fhahn wrote:
> `emplace_back`?
`TinyPtrVector` does not have 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 =
----------------
fhahn wrote:
> 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!
Yep


================
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:
> 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.
Yep, I will undo it.


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

https://reviews.llvm.org/D99354