[PATCH] D95245: [SVE] Add support for scalable vectorization of loops with int/fast FP reductions

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1103
                                            TTI::ReductionFlags Flags) const {
+  if (isa<ScalableVectorType>(Ty))
+    return true;
----------------
david-arm wrote:
> sdesmalen wrote:
> > david-arm wrote:
> > > Just a thought - if we're excluding FMul from reductions is it worth having an assert here that the op is not fmul?
> > The same should hold for integer Mul.
> > 
> > nit: you can better add that to the switch statement below as:
> >   case Instruction::Mul:
> >   case Instruction::FMul:
> >     assert(!isa<ScalableVectorType>(Ty) && "Unexpected ...");
> >     LLVM_FALLTHROUGH;
> >   case Instruction::Fadd:
> >   ...
> > 
> Hi @sdesmalen, just for information the reason I'd asked for an assert here is that if we're still intending to create a target reduction intrinsic at this point with a mul or fmul then something has gone badly wrong and is almost certainly a bug. This is because this function is only ever called at the point where you've already decided that it's legal to reduce a scalable mul operation. The two places where this is called are from SLPVectorizer.cpp:createSimpleTargetReduction and InnerLoopVectorizer::fixReduction (via createTargetReduction).
Sorry, please ignore my comment! For some reason I hadn't seen the assert in there.


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

https://reviews.llvm.org/D95245