[llvm] [VPlan] Permit licm-sinking recipes with no users (PR #189957)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-transforms

Author: Ramkumar Ramachandra (artagnon)

<details>
<summary>Changes</summary>

The patch is a preparatory step for sinking invariant stores in licm. We have extended cannotHoistOrSink to accept a flag indicating whether we are sinking, in order to prevent sinking assumes: without this change, there is a bad test update in tree. Unfortunately, as the only possible instructions without users that will not be trivially dce'd, and on which the vectorizer will not bail out are, are stores and assumes: since we forbid assume-sinking, and store sinking will only be done in a follow-up, we have no test changes to show. The patch has not been marked as an NFC, because the additonal flag in cannotHoistOrSink is used in more sinking transforms, which would prevent assumes from being sunk.

---
Full diff: https://github.com/llvm/llvm-project/pull/189957.diff


1 Files Affected:

- (modified) llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp (+26-30) 


``````````diff
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index 90685dcb7ed63..c3c5abb02cc1d 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -291,11 +291,13 @@ collectGroupedReplicateMemOps(
 
 /// Return true if we do not know how to (mechanically) hoist or sink \p R out
 /// of a loop region.
-static bool cannotHoistOrSinkRecipe(const VPRecipeBase &R) {
+static bool cannotHoistOrSinkRecipe(const VPRecipeBase &R,
+                                    bool Sinking = false) {
   // Assumes don't alias anything or throw; as long as they're guaranteed to
-  // execute, they're safe to hoist.
+  // execute, they're safe to hoist. They should however not be sunk, as it
+  // would destroy information.
   if (match(&R, m_Intrinsic<Intrinsic::assume>()))
-    return false;
+    return Sinking;
 
   // TODO: Relax checks in the future, e.g. we could also hoist reads, if their
   // memory location is not modified in the vector loop.
@@ -325,7 +327,8 @@ static bool sinkScalarOperands(VPlan &Plan) {
     if (!isa<VPReplicateRecipe, VPScalarIVStepsRecipe>(Candidate))
       return;
 
-    if (Candidate->getParent() == SinkTo || cannotHoistOrSinkRecipe(*Candidate))
+    if (Candidate->getParent() == SinkTo ||
+        cannotHoistOrSinkRecipe(*Candidate, /*Sinking=*/true))
       return;
 
     if (auto *RepR = dyn_cast<VPReplicateRecipe>(Candidate))
@@ -2337,7 +2340,7 @@ sinkRecurrenceUsersAfterPrevious(VPFirstOrderRecurrencePHIRecipe *FOR,
         VPDT.properlyDominates(Previous, SinkCandidate))
       return true;
 
-    if (cannotHoistOrSinkRecipe(*SinkCandidate))
+    if (cannotHoistOrSinkRecipe(*SinkCandidate, /*Sinking=*/true))
       return false;
 
     WorkList.push_back(SinkCandidate);
@@ -2708,13 +2711,12 @@ static void licm(VPlan &Plan) {
 #ifndef NDEBUG
   VPDominatorTree VPDT(Plan);
 #endif
-  // Sink recipes with no users inside the vector loop region if all users are
-  // in the same exit block of the region.
-  // TODO: Extend to sink recipes from inner loops.
+  // Sink recipes with in the vector loop region to a unique successor of the
+  // loop region.
   for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>(
            vp_post_order_shallow(LoopRegion->getEntry()))) {
     for (VPRecipeBase &R : make_early_inc_range(reverse(*VPBB))) {
-      if (cannotHoistOrSinkRecipe(R))
+      if (cannotHoistOrSinkRecipe(R, /*Sinking=*/true))
         continue;
 
       if (auto *RepR = dyn_cast<VPReplicateRecipe>(&R)) {
@@ -2732,42 +2734,36 @@ static void licm(VPlan &Plan) {
       // TODO: Use R.definedValues() instead of casting to VPSingleDefRecipe to
       // support recipes with multiple defined values (e.g., interleaved loads).
       auto *Def = cast<VPSingleDefRecipe>(&R);
-      // Skip recipes without users as we cannot determine a sink block.
-      // TODO: Clone sinkable recipes without users to all exit blocks to reduce
-      // their execution frequency.
-      if (Def->getNumUsers() == 0)
-        continue;
 
+      // Cannot sink the recipe if the user is defined in a loop region or a
+      // non-successor of the vector loop region. Cannot sink if user is a phi
+      // either.
       VPBasicBlock *SinkBB = nullptr;
-      // Cannot sink the recipe if any user
-      //  * is defined in any loop region, or
-      //  * is a phi, or
-      //  * multiple users in different blocks.
-      if (any_of(Def->users(), [&SinkBB](VPUser *U) {
+      if (any_of(Def->users(), [&SinkBB, &LoopRegion](VPUser *U) {
             auto *UserR = cast<VPRecipeBase>(U);
             VPBasicBlock *Parent = UserR->getParent();
-            // TODO: If the user is a PHI node, we should check the block of
-            // incoming value. Support PHI node users if needed.
-            if (UserR->isPhi() || Parent->getEnclosingLoopRegion())
-              return true;
             // TODO: Support sinking when users are in multiple blocks.
             if (SinkBB && SinkBB != Parent)
               return true;
             SinkBB = Parent;
-            return false;
+            // TODO: If the user is a PHI node, we should check the block of
+            // incoming value.
+            return UserR->isPhi() || Parent->getEnclosingLoopRegion() ||
+                   Parent->getSinglePredecessor() != LoopRegion;
           }))
         continue;
 
-      // Only sink to dedicated exit blocks of the loop region.
-      if (SinkBB->getSinglePredecessor() != LoopRegion)
+      // Attempt to set SinkBB to the LoopRegion's single successor, if one
+      // wasn't found.
+      if (!SinkBB && !(SinkBB = cast_or_null<VPBasicBlock>(
+                           LoopRegion->getSingleSuccessor())))
         continue;
 
-      // TODO: This will need to be a check instead of a assert after
-      // conditional branches in vectorized loops are supported.
+      // This will need to be a check instead of a assert after conditional
+      // branches in vectorized loops are supported.
       assert(VPDT.properlyDominates(VPBB, SinkBB) &&
              "Defining block must dominate sink block");
-      // TODO: Clone the recipe if users are on multiple exit paths, instead of
-      // just moving.
+      // TODO: Clone the recipe if users are on multiple exit paths.
       Def->moveBefore(*SinkBB, SinkBB->getFirstNonPhi());
     }
   }

``````````

</details>


https://github.com/llvm/llvm-project/pull/189957