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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour 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:
> > 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?


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

https://reviews.llvm.org/D89322