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

[Dave Green via Phabricator via llvm-commits]

dmgreen 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
----------------
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.


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

https://reviews.llvm.org/D89322