[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;
----------------
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).


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

https://reviews.llvm.org/D95245