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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:283
+      std::pair<VPlanPtr, SmallVector<std::pair<VectorizationFactor, bool>>>, 4>
+      VPlans;
 
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > fhahn wrote:
> > > > fhahn wrote:
> > > > > Ayal wrote:
> > > > > > 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.
> > > > > Thanks, I updated the code to use a separate mapping from VPlan->vector of pair<VectorizationFactor, bool>. This ensures that the costs are associated with a specific VPlan. WDYT?
> > > > > 
> > > > > As for the IsVec field, I am going to investigate if this is actually needed or if we can get the information from the existing VPlan.
> > > > > As for the IsVec field, I am going to investigate if this is actually needed or if we can get the information from the existing VPlan.
> > > > 
> > > > We don't need it at this stage any longer, we can instead just avoid adding such VFs to CandidateVFs. Removed the boolean flag from LVP.
> > > >> 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?
> > > 
> > > > Thanks, I updated the code to use a separate mapping from VPlan->vector of pair<VectorizationFactor, bool>. This ensures that the costs are associated with a specific VPlan. WDYT?
> > > 
> > > A single cost is currently computed and associated with a specific VF as indicated by `expectedCost()`, `select[Epilogue]VectorizationFactor()` and LVP::plan()'s `CandidateVF`s; a single Plan is expected for each VF including 1 and BestVF. So LVP can hold all VectorizationFactors in a vector for now, or in a set indexed by Width, regardless of their Plans.
> > > Can revisit this if/when the Plan associated with each VectorizationFactor and/or the set of VectorizationFactors corresponding to a Plan's `VFs` are needed. Sounds reasonable?
> > Follow-up patches will break the 1-1 mapping from VFs->Cost e.g. when planning with multiple cost models there may be different plans for the same VFs but with different costs. For that, the VPlan->costs mapping seems to be required; happy to have VF->Cost mapping as intermediate step, but it might lead to less churn to use the Plan->costs mapping in this patch already?
> This may indeed lead to less churn, but it would be good to reason about which mapping is needed along with a usage that needs it, such as planning with multiple cost models. Given that CM currently holds ProfitableVFs as a vector of VectorizationFactors, suffice for this patch to focus on moving such a vector to LVP?
Good point, I split off the parts of only moving the selectVectorizationFactor logic to LVP to D150197.


I'll update this patch once that is in, which should also drastically reduce the diff here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143938