[PATCH] D75069: [LoopVectorizer] Inloop vector reductions

[Dave Green via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:238
   VPTransformState(unsigned VF, unsigned UF, LoopInfo *LI, DominatorTree *DT,
-                   IRBuilder<> &Builder, VectorizerValueMap &ValueMap,
-                   InnerLoopVectorizer *ILV, VPCallback &Callback)
-      : VF(VF), UF(UF), Instance(), LI(LI), DT(DT), Builder(Builder),
+                   const TargetTransformInfo *TTI, IRBuilder<> &Builder,
+                   VectorizerValueMap &ValueMap, InnerLoopVectorizer *ILV,
----------------
gilr wrote:
> dmgreen wrote:
> > Ayal wrote:
> > > Too bad this requires passing TTI through the State everywhere.
> > > Perhaps storing TTI in the recipe would be somewhat better.
> > I've changed it to be stored there. It does mean multiple things are holding TTI. Let me know what you think.
> It seems that TTI is only used later for deciding whether to use a shuffle sequence or an intrinsic based on data available during planning. If so, then it would be best if the Planner calls TTI->useReductionIntrinsic() and records that boolean decision in the Recipe. This is also required in order to estimate in-loop reduction cost. This could be done separately.
Do you mean to change the interface to createTargetReduction, to take a bool instead? Yeah I think that sounds good. I'd prefer to do it as a separate review as it does involve changing the interface. I will put a patch together.

I was imagining that we would change the cost to use getArithmeticReductionCost, which hopefully handles the details of how the target lowers reductions. I haven't looked deeply into the details yet though. That is on the list of things to do.


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

https://reviews.llvm.org/D75069