[VPlan] Update getBestPlan to return VF, use also for epilogue vec. #98821

fhahn · 2024-07-14T16:27:06Z

Update getBestPlan to return the VF alongside the best plan instead of restricting the returned plan's VFs to the best VF.

This is allows using getBestPlan to also get the best VPlan for epilogue vectorization. As the same plan may be used to vectorize both the main and epilogue loop, restricting the VF of the best plan would cause issues.

Update getBestPlan to return the VF alongside the best plan instead of restricting the returned plan's VFs to the best VF. This is allows using getBestPlan to also get the best VPlan for epilogue vectorization. As the same plan may be used to vectorize both the main and epilogue loop, restricting the VF of the best plan would cause issues.

llvmbot · 2024-07-14T16:27:35Z

@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Update getBestPlan to return the VF alongside the best plan instead of restricting the returned plan's VFs to the best VF.

This is allows using getBestPlan to also get the best VPlan for epilogue vectorization. As the same plan may be used to vectorize both the main and epilogue loop, restricting the VF of the best plan would cause issues.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h (+3-2)
(modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+8-12)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h b/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
index c63cf0c37f2f9..62b5d270e2f04 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
@@ -365,8 +365,9 @@ class LoopVectorizationPlanner {
   /// Return the best VPlan for \p VF.
   VPlan &getBestPlanFor(ElementCount VF) const;
 
-  /// Return the most profitable plan and fix its VF to the most profitable one.
-  VPlan &getBestPlan() const;
+  /// Return the most profitable vectorization factor together with the most
+  /// profitable plan containing that vectorization factor.
+  std::pair<ElementCount, VPlan &> getBestPlan() const;
 
   /// Generate the IR code for the vectorized loop captured in VPlan \p BestPlan
   /// according to the best selected \p VF and  \p UF.
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 7d37d67cde29c..2ea136c7ebc48 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -7423,11 +7423,11 @@ InstructionCost LoopVectorizationPlanner::cost(VPlan &Plan,
   return Cost;
 }
 
-VPlan &LoopVectorizationPlanner::getBestPlan() const {
+std::pair<ElementCount, VPlan &> LoopVectorizationPlanner::getBestPlan() const {
   // If there is a single VPlan with a single VF, return it directly.
   VPlan &FirstPlan = *VPlans[0];
   if (VPlans.size() == 1 && size(FirstPlan.vectorFactors()) == 1)
-    return FirstPlan;
+    return {*FirstPlan.vectorFactors().begin(), FirstPlan};
 
   VPlan *BestPlan = &FirstPlan;
   ElementCount ScalarVF = ElementCount::getFixed(1);
@@ -7466,8 +7466,7 @@ VPlan &LoopVectorizationPlanner::getBestPlan() const {
       }
     }
   }
-  BestPlan->setVF(BestFactor.Width);
-  return *BestPlan;
+  return {BestFactor.Width, *BestPlan};
 }
 
 VPlan &LoopVectorizationPlanner::getBestPlanFor(ElementCount VF) const {
@@ -10287,6 +10286,11 @@ bool LoopVectorizePass::processLoop(Loop *L) {
     } else {
       // If we decided that it is *legal* to vectorize the loop, then do it.
 
+      const auto &[Width, BestPlan] = LVP.getBestPlan();
+      LLVM_DEBUG(dbgs() << "VF picked by VPlan cost model: " << Width << "\n");
+      assert(VF.Width == Width &&
+             "VPlan cost model and legacy cost model disagreed");
+
       // Consider vectorizing the epilogue too if it's profitable.
       VectorizationFactor EpilogueVF =
           LVP.selectEpilogueVectorizationFactor(VF.Width, IC);
@@ -10395,14 +10399,6 @@ bool LoopVectorizePass::processLoop(Loop *L) {
                                VF.MinProfitableTripCount, IC, &LVL, &CM, BFI,
                                PSI, Checks);
 
-        VPlan &BestPlan = LVP.getBestPlan();
-        assert(size(BestPlan.vectorFactors()) == 1 &&
-               "Plan should have a single VF");
-        ElementCount Width = *BestPlan.vectorFactors().begin();
-        LLVM_DEBUG(dbgs() << "VF picked by VPlan cost model: " << Width
-                          << "\n");
-        assert(VF.Width == Width &&
-               "VPlan cost model and legacy cost model disagreed");
         LVP.executePlan(Width, IC, BestPlan, LB, DT, false);
         ++LoopsVectorized;

rengolin

Looks like a simple cleanup, LGTM, but please wait a day in case others have comments.

…-epilogue

ayalz

Some further thoughts.

ayalz · 2024-07-21T05:56:03Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

+      ElementCount Width = VF.Width;
+      std::optional<VPlan *> VPlanFromVPCost;
+      if (!UseLegacyCostModel) {
+        const auto &[VPWidth, Plan] = LVP.getBestPlan();


It is a bit confusing to claim that getBestPlan() is also used for epilogue vectorization, given that Plan is soon overwritten by BestPlan, which is later overwritten by BestMainPlan (and BestEpiPlan) for epilogue vectorization. Could (some of) this redundancy be eliminated.

The current assertion checks best VF's rather than best Plans, verifying that LVP.getBestVF() equals the VF.Width of LVP.plan(), when vectorizing the main loop only; i.e., avoids checking the VF's of the main or epilog loops when doing epilog vectorization, right?

I removed the temporary UseLegacyCostModel option now that we branched for 19.x, so this is now simpler again

ayalz · 2024-07-21T06:10:13Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

+        Width = VPWidth;
+        assert(VF.Width == Width &&


Suggested change

Width = VPWidth;

assert(VF.Width == Width &&

assert(VF.Width == VPWidth &&

A bit confusing to have both VPWidth and VF.Width, perhaps clearer to rename the former VPlanWidth. Width is expected to be equal to both.

ayalz · 2024-07-21T13:10:06Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

@@ -7496,8 +7496,7 @@ VPlan &LoopVectorizationPlanner::getBestPlan() const {
      }
    }
  }
-  BestPlan->setVF(BestFactor.Width);


Agreed it is better to avoid restricting BestPlan's VF's to BestFactor.Width alone, thereby discarding all other associated VF's, one of which may be reuseful for subsequent epilog vectorization. This was the original intent of LVP's getBestPlan(). Cloning could be employed to retain the other VF's, but may be redundant - in case they are not needed.

ayalz · 2024-07-21T13:23:29Z

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h

-  VPlan &getBestPlan() const;
+  /// Return the most profitable vectorization factor together with the most
+  /// profitable plan containing that vectorization factor.
+  std::pair<ElementCount, VPlan &> getBestPlan() const;


There is an underlying assumption that, at-least at this stage, there is at most a single VPlan per VF.

Perhaps it would be clearer to have getBestVF() complement getPlanFor(VF) - rather than getBestPlan() and getBestPlanFor(VF), which is more aligned with LVP's plan()? This would require two calls instead of a single getBestPlan() with two return values, but getPlanFor(VF) could be a simple map retrieval.

The VPlan per scalar VF deserves perhaps to be held separately, corresponding to the original scalar loop, w/ or w/o undergoing unrolling(?)

Updated to introduce getBestVF and use it in combination with getBestPlanFor, without a map yet.

ayalz · 2024-07-21T13:28:01Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

  // If there is a single VPlan with a single VF, return it directly.
  VPlan &FirstPlan = *VPlans[0];
  if (VPlans.size() == 1 && size(FirstPlan.vectorFactors()) == 1)
-    return FirstPlan;
+    return {*FirstPlan.vectorFactors().begin(), FirstPlan};

  VPlan *BestPlan = &FirstPlan;


Note (independent of this patch) a slight discrepancy in recording an initial {BestPlan, BestFactor} pair as {FirstPlan, ScalarCost or Max cost}, rather than having BestFactor be a factor of BestPlan. Somewhat more consistent to set BestPlan initially to ScalarPlan instead?

…-epilogue

ayalz

LGTM, thanks, adding last few minor comments.

ayalz · 2024-07-24T19:08:02Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

@@ -7178,12 +7177,10 @@ VPlan &LoopVectorizationPlanner::getBestPlan() const {
      VectorizationFactor CurrentFactor(VF, Cost, ScalarCost);
      if (isMoreProfitable(CurrentFactor, BestFactor)) {
        BestFactor = CurrentFactor;
-        BestPlan = &*P;


nit: can now also drop the {}

Done, thanks!

ayalz · 2024-07-24T19:10:15Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

+      VPlan &BestPlan = LVP.getBestPlanFor(BestVF);
+      assert(BestVF.isScalar() &&
             "VPlan cost model and legacy cost model disagreed");


Suggested change

VPlan &BestPlan = LVP.getBestPlanFor(BestVF);

assert(BestVF.isScalar() &&

"VPlan cost model and legacy cost model disagreed");

assert(BestVF.isScalar() &&

"VPlan cost model and legacy cost model disagreed");

VPlan &BestPlan = LVP.getBestPlanFor(BestVF);

Reordered, thanks!

ayalz · 2024-07-24T19:14:30Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

@@ -9985,15 +9983,20 @@ bool LoopVectorizePass::processLoop(Loop *L) {
    } else {
      // If we decided that it is *legal* to vectorize the loop, then do it.

+      ElementCount BestVF = LVP.getBestVF();
+      VPlan &BestPlan = LVP.getBestPlanFor(BestVF);


Suggested change

VPlan &BestPlan = LVP.getBestPlanFor(BestVF);

better set closer to where its used below.

The epilogue code now also uses the fetched plan here to be duplicated for the main loop, so I retained it here, as it shared by both sides of the if

ayalz · 2024-07-24T19:16:17Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

                               VF.MinProfitableTripCount, IC, &LVL, &CM, BFI,
                               PSI, Checks);
-        LVP.executePlan(Width, IC, BestPlan, LB, DT, false);
+        LVP.executePlan(BestVF, IC, BestPlan, LB, DT, false);


Suggested change

LVP.executePlan(BestVF, IC, BestPlan, LB, DT, false);

VPlan &BestPlan = LVP.getBestPlanFor(BestVF);

LVP.executePlan(BestVF, IC, BestPlan, LB, DT, false);

Left at the original position as it is shared by both sides of the if

ayalz · 2024-07-24T19:18:38Z

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h

  /// Return the best VPlan for \p VF.
  VPlan &getBestPlanFor(ElementCount VF) const;


Suggested change

/// Return the best VPlan for \p VF.

VPlan &getBestPlanFor(ElementCount VF) const;

/// Return the VPlan for \p VF.

VPlan &getPlanFor(ElementCount VF) const;

would be more accurate, independent of this patch.

Will do separately, thanks!

ayalz · 2024-07-24T19:24:44Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

-      assert(BestPlan.hasScalarVFOnly() &&
+      ElementCount BestVF = LVP.getBestVF();
+      VPlan &BestPlan = LVP.getBestPlanFor(BestVF);
+      assert(BestVF.isScalar() &&
             "VPlan cost model and legacy cost model disagreed");
      LVP.executePlan(VF.Width, IC, BestPlan, Unroller, DT, false);


Suggested change

LVP.executePlan(VF.Width, IC, BestPlan, Unroller, DT, false);

LVP.executePlan(BestVF, IC, BestPlan, Unroller, DT, false);

following the typical pattern of having LVP.executePlan(VF, IC, Plan, ...) follow VPlan &Plan = LVP.getPlanFor(VF).

Fixed, thanks!

…-epilogue

Cleanup to make things consistent in preparation for #98821.

…-epilogue

ayalz · 2024-07-26T12:57:55Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

        EpilogueVectorizerMainLoop MainILV(L, PSE, LI, DT, TLI, TTI, AC, ORE,
                                           EPI, &LVL, &CM, BFI, PSI, Checks);

        assert(EPI.MainLoopVF == VF.Width && "VFs must match");
-        std::unique_ptr<VPlan> BestMainPlan(
-            LVP.getBestPlanFor(VF.Width).duplicate());
+        std::unique_ptr<VPlan> BestMainPlan(BestPlan.duplicate());


Ah, right, BestPlan is now used here.

ayalz · 2024-07-26T13:02:06Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

      if (EpilogueVF.Width.isVector()) {

        // The first pass vectorizes the main loop and creates a scalar epilogue
        // to be vectorized by executing the plan (potentially with a different
        // factor) again shortly afterwards.
-        EpilogueLoopVectorizationInfo EPI(VF.Width, IC, EpilogueVF.Width, 1);
+        EpilogueLoopVectorizationInfo EPI(BestVF, IC, EpilogueVF.Width, 1);
        EpilogueVectorizerMainLoop MainILV(L, PSE, LI, DT, TLI, TTI, AC, ORE,
                                           EPI, &LVL, &CM, BFI, PSI, Checks);

        assert(EPI.MainLoopVF == VF.Width && "VFs must match");


Suggested change

assert(EPI.MainLoopVF == VF.Width && "VFs must match");

assert(EPI.MainLoopVF == BestVF && "VFs must match");

or dropped - given that this holds by construction of EPI?

Maybe worth revisiting the use of EPI throughout, separately, as it leads to an inflation of VF's, a redundant setting of EpilogueUF asserted to be 1, a confusing resetting of EPI.MainLoopVF = EPI.EpilogueVF; below.

Yep it is not needed any longer, removed in 5a9b9ef

Maybe worth revisiting the use of EPI throughout, separately, as it leads to an inflation of VF's, a redundant setting of EpilogueUF asserted to be 1, a confusing resetting of EPI.MainLoopVF = EPI.EpilogueVF; below.

Yes, this should be untangled.

fhahn requested review from rengolin, ayalz and aniragil July 14, 2024 16:27

llvmbot added vectorizers llvm:transforms labels Jul 14, 2024

rengolin approved these changes Jul 14, 2024

View reviewed changes

fhahn added 2 commits July 20, 2024 10:29

Merge remote-tracking branch 'origin/main' into vplan-getbestplan-for…

59101ce

…-epilogue

!fixup update after updating to latest main.

69fd80e

llvmbot added the backend:RISC-V label Jul 20, 2024

ayalz reviewed Jul 21, 2024

View reviewed changes

fhahn added 2 commits July 24, 2024 18:07

Merge remote-tracking branch 'origin/main' into vplan-getbestplan-for…

7e369ba

…-epilogue

!fixup introduce getBestVF.

28f2930

ayalz approved these changes Jul 24, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into vplan-getbestplan-for…

e7705a8

…-epilogue

fhahn added a commit that referenced this pull request Jul 26, 2024

[VPlan] Consistently use VF.Width to getting plan for main loop VF (NFC)

8b02f31

Cleanup to make things consistent in preparation for #98821.

fhahn added 2 commits July 26, 2024 12:08

Merge remote-tracking branch 'origin/main' into vplan-getbestplan-for…

31731b3

…-epilogue

!fixup address comments, thanks!

52f032c

ayalz reviewed Jul 26, 2024

View reviewed changes

fhahn merged commit 67a55e0 into llvm:main Jul 26, 2024
7 checks passed

fhahn deleted the vplan-getbestplan-for-epilogue branch July 26, 2024 13:06

	Width = VPWidth;
	assert(VF.Width == Width &&
	assert(VF.Width == VPWidth &&

	LVP.executePlan(BestVF, IC, BestPlan, LB, DT, false);
	VPlan &BestPlan = LVP.getBestPlanFor(BestVF);
	LVP.executePlan(BestVF, IC, BestPlan, LB, DT, false);

		/// Return the best VPlan for \p VF.
		VPlan &getBestPlanFor(ElementCount VF) const;

	LVP.executePlan(VF.Width, IC, BestPlan, Unroller, DT, false);
	LVP.executePlan(BestVF, IC, BestPlan, Unroller, DT, false);

	assert(EPI.MainLoopVF == VF.Width && "VFs must match");
	assert(EPI.MainLoopVF == BestVF && "VFs must match");

[VPlan] Update getBestPlan to return VF, use also for epilogue vec. #98821

[VPlan] Update getBestPlan to return VF, use also for epilogue vec. #98821

Uh oh!

Conversation

fhahn commented Jul 14, 2024

Uh oh!

llvmbot commented Jul 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rengolin left a comment

Choose a reason for hiding this comment

Uh oh!

ayalz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayalz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jul 14, 2024 •

edited

Loading