[PATCH] D89322: [LV] Initial VPlan cost modelling

[Andrei Elovikov via Phabricator via llvm-commits]

a.elovikov added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:372
+/// holds the CostModel and Legailty pointers, which can be expanded as needed.
+struct VPCostContext {
+  /// The original CostModel, which is currently used for getting instruction
----------------
dmgreen wrote:
> bmahjour wrote:
> > dmgreen wrote:
> > > bmahjour wrote:
> > > > The cost-model is conceptually the structure that knows all about calculating costs. Instead of packaging the cost-model and legality and send it to the VPlan, wouldn't it make more sense to send the VPlan to the cost-model as it already has access to legality?
> > > The idea, as far as I understand, is that just as the execution of a vplan is separated into the recipes that make it up, so should the cost model. All the CM.getInstructionCost(..) methods will be replaced by the code that make them up - VPInterleaveRecipe will know how to cost interleaving groups, VPReductions will know how to cost reduction, VPWidenRecipes will be able to handle widened instructions etc
> > > 
> > > The LoopVectorizationCostModel is the old monolithic way of doing things, which vplan is trying to move away from. This is only the first step of trying to clean that up..
> > > 
> > > As for this struct specifically, yeah it doesn't feel like the best. I was trying to follow VPTransformState but it doesn't contain much in it at the moment. Plus I apparently misspelt analysis.
> > Ok, if the idea is to eventually remove the `LoopVectorizationCostModel` in the future, this change makes sense as a transitional step. 
> > By the way, I find it strange that we rely on `LoopVectorizationLegality` to get information about the cost! Looks like we only need `getWidestInductionType` from it, so can we put that in the context instead of the whole class?
> Ah Good point. Some of this code was added and removed along the way.
> 
> You are right that the Legality isn't really needed much at the moment. We may need it for some things in the future, but if we do can address those as they come up.
> Instead of packaging the cost-model and legality and send it to the VPlan, wouldn't it make more sense to send the VPlan to the cost-model...

My +1 for putting CM code outside the VPlan itself. I prefer to see VPlan as a kind of IR in itself and hence it would make sense to keep it similar to LLVM IR CostModeling via separate class. In fact, I'd rather move `::execute` methods out of VPlan as well.

Moving the cost modeling code out of VPlan classes might also make it easier to prototype/implement some advanced heuristics that would work on the basicblock/whole vplan level (i.e. some kinds of register pressure/spills/fills estimates) by just putting all the code locally and grouped by different level of details we want to account for. We might also consider cost modeling for either throughput or latency depending on the trip counts. IMO, that would also be easier to code if we'd have a separate class.


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

https://reviews.llvm.org/D89322