[PATCH] D105432: [Analysis] Add simple cost model for strict (in-order) reductions

Dave Green via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 6 07:27:36 PDT 2021


dmgreen added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:2085
+  InstructionCost
+  getArithmeticStrictReductionCost(unsigned Opcode, VectorType *Ty,
+                                   TTI::TargetCostKind CostKind) {
----------------
david-arm wrote:
> dmgreen wrote:
> > david-arm wrote:
> > > dmgreen wrote:
> > > > sdesmalen wrote:
> > > > > Can you unify the interface for getArithmeticReductionCost to take an enum which specifies the kind of reduction we want: tree, pair-wise or in-order?
> > > > > If there is only a single `getReductionCost` interface where the parameters dictate having to specify what cost is being asked for, than that's difficult to get wrong (you need to fill in the parameters), but if there's another slightly more specialized variant available, people may not realise it exists and just call the most basic cost interface instead.
> > > > Oh you got here first.  :)
> > > > 
> > > > I think we can possibly remove IsPairwise entirely. It seems to be only used in a single place nowadays that doesn't seem to me like it would be giving very good cost estimates.
> > > Hi @dmgreen, it turns out removing the pairwise form breaks *lots* of tests. :) I think it's still needed because `TTI::matchVectorReduction` returns the pairwise form for a lot of cases.
> > I happened to be looking at if we can remove it too. Are you assuming that IsPairwise = false? The SLP vectorizer works differently now to how it did in the past.
> > 
> > I can put a patch for that if you like. There is one cost test that changes, but it seems to me like they would be OK to change, and the code creating the costs removed.
> > 
> > It is at least a (mostly) separate issue from this patch.
> The problem is that TTI::matchVectorReduction sets IsPairwise to true in some tests run by "make check-all" causing them to fail. I'm not sure where exactly this function gets called from, but I can't just remove the IsPairwise option as part of this patch. I imagine it requires some investigation to work out why the callers need that? However, if you've already got a patch that removes this then I can just rebase on top of it!
Yeah, leave that bit to me. Consider it to be removed, but I think the patches can end up happening in either order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105432



More information about the llvm-commits mailing list