[PATCH] D143938: [VPlan] Compute costs for plans directly after construction.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Good move towards getting Planning and eventually VPlans involved with their cost.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:283
+      std::pair<VPlanPtr, SmallVector<std::pair<VectorizationFactor, bool>>>, 4>
+      VPlans;
 
----------------
Perhaps the following would be better, at-least when first moving VectorizationFactors from CM to LVP: keep `VPlans` as a vector of all VPlanPtrs, as its name suggests; hold all VectorizationFactors in a separate vector or set indexed by their ElementCount Width (as in LVP::plan()'s CandidateVFs)? Can also hold a map associating each VectorizationFactor with its VPlan, if needed; is the reverse mapping useful?

(Independent of this patch): Hold the boolean IsVec as a field inside VectorizationFactor, rename VectorizationFactor more accurately.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:287
   VPBuilder Builder;
 
+  using InstructionVFPair = std::pair<Instruction *, ElementCount>;
----------------
/// ...


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:328
   /// Look through the existing plans and return true if we have one with all
   /// the vectorization factors in question.
   bool hasPlanWithVF(ElementCount VF) const {
----------------
nit (independent of this patch): "all the vectorization factors in question"?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:347-350
+  /// \return The most profitable vectorization factor and the cost of that VF.
+  /// This method checks every VF in \p CandidateVFs. If UserVF is not ZERO
+  /// then this vectorization factor will be selected if vectorization is
+  /// possible.
----------------
nit: comment deserves update - CandidateVFs is no longer a parameter. (Independent of this patch:) UserVF is irrelevant, behavior of ForceVectorization should be explained.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:352
+  VectorizationFactor selectVectorizationFactor();
+
+  VectorizationFactor
----------------
/// ... (independent of this patch)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:390
                                   ElementCount MinVF);
+
+  std::optional<unsigned> getVScaleForTuning() const;
----------------
/// ... (independent of this patch)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:404
+  /// false otherwise.
+  /// \p VF is the vectorization factor chosen for the original loop.
+  bool isEpilogueVectorizationProfitable(const ElementCount VF) const;
----------------
nit (independent of this patch): above needs L but below needs not?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5453
+    auto RTCostA = CostA * divideCeil(MaxTripCount, A.Width.getFixedValue());
+    auto RTCostB = CostB * divideCeil(MaxTripCount, B.Width.getFixedValue());
+    return RTCostA < RTCostB;
----------------
nit (independent of this patch): cache RTCost in VectorizationFactor if TripCount is known and Width isn't scalable? 


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5475
+  //      (CostA / A.Width) < (CostB / B.Width)
+  // <=>  (CostA * B.Width) < (CostB * A.Width)
+  return (CostA * EstimatedWidthB) < (CostB * EstimatedWidthA);
----------------
nit (independent of this patch): comment for multiplying instead of dividing should appear above, before favoring scalable A.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5513
+      return !isMoreProfitable(Candidate.first, ChosenFactor);
+    });
 
----------------
This is independent of Plan, right?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5523
+            << "LV: Not considering vector loop of width " << Candidate.Width
+            << " because it will not generate any vector instructions.\n");
+        continue;
----------------
Thought (independent of patch): would a Plan/VectorizationFactor of Width VF with !HasVec be similar to a Plan of Width 1 and UF=VF? If so, is HasVec redundant (when !ForceVectorization)?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5659
+           ElementCount::isKnownLT(NextVF.Width, MainLoopVF)) &&
+          (Result.Width.isScalar() || isMoreProfitable(NextVF, Result)))
+        Result = NextVF;
----------------
nit: can assert there's a Plan that has NextVF.Width?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6362
 
-LoopVectorizationCostModel::VectorizationCostTy
-LoopVectorizationCostModel::expectedCost(
+VectorizationCostTy LoopVectorizationCostModel::expectedCost(
     ElementCount VF, SmallVectorImpl<InstructionVFPair> *Invalid) {
----------------
nit: change independent of this patch?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6814
 
-LoopVectorizationCostModel::VectorizationCostTy
+VectorizationCostTy
 LoopVectorizationCostModel::getInstructionCost(Instruction *I,
----------------
ditto


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8752
+        "ConditionalStore", ORE, OrigLoop);
+    VPlans.clear();
+  }
----------------
nit: also prevents unrolling, hence no need to retain even the scalar VPlan of VF=1 for unrolling UF>1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143938