[PATCH] D100121: [LV] Let selectVectorizationFactor reason directly on VectorizationFactor.

Thu Apr 15 07:59:58 PDT 2021

bmahjour added inline comments.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:203
+  // Returns true if this VectorizationFactor is more profitable than \p rhs.
+  bool isMoreProfitableThan(const VectorizationFactor &rhs) const {
+    InstructionCost::CostType ThisCost = *Cost.getValue();
----------------
dmgreen wrote:
> sdesmalen wrote:
> > dmgreen wrote:
> > > rhs -> RHS
> > > 
> > > If the backend knew that the SVE vector length was 256, as opposed 128, how would it best communicate that information to here?
> > Doing it here without context would be a bit difficult, as it needs information from CostModel/TargetTransformInfo in order to know which one to favour. Hence my reason to initially create this method in the LV CostModel (see the previous revision https://reviews.llvm.org/D100121?id=336138), so it would mean either passing that information separately, or moving it to the other class again.
> Yeah, I think that makes sense. If we have a good reason to put it into the cost model (and @bmahjour doesn't object), then keeping it in the cost model so it could have access to TTI etc would make sense to me.
I don't have any objections to moving it back, if it makes information more accessible for future extensions.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100121