[PATCH] D87391: [Intrinsics] define semantics for experimental fmax/fmin vector reductions

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/test/CodeGen/Thumb2/mve-vecreduce-fminmax.ll:247
+; CHECK-FP-NEXT:    vminnm.f32 s0, s0, s1
+; CHECK-FP-NEXT:    vminnm.f32 s0, s0, s4
 ; CHECK-FP-NEXT:    bx lr
----------------
spatel wrote:
> dmgreen wrote:
> > dmgreen wrote:
> > > spatel wrote:
> > > > nikic wrote:
> > > > > This lowering looks incorrect for the case where both elements are NaN. We'll fold to +INF then. We probably have an expansion that assumes +INF is a neutral element for fminnum, but it isn't in the presence of NaNs :/
> > > > Good catch - something in vector legalization does that:
> > > >         t15: v4f32 = insert_vector_elt t12, ConstantFP:f32<INF>, Constant:i32<2>
> > > >       t17: v4f32 = insert_vector_elt t15, ConstantFP:f32<INF>, Constant:i32<3>
> > > >     t18: f32 = vecreduce_fmin t17
> > > > 
> > > I originally thought this was because we don't go through ExpandReductions, widening them in ISel instead. They do look like they get padded with +/- Inf in that case.
> > > 
> > > But we do expand pre-isel if NoNan isn't present in shouldExpandReduction. I looks like some of the expansion of min/max is unconditionally setting fast flags in llvm::createMinMaxOp. Unless I'm mistaken.
> > > 
> > > The padding with +/- inf is likely a problem on it's own right too.
> > Oh I see you are change how that works. It sounds like shouldExpandReduction could be updated then?
> Hmm...not sure.
> It's not clear to me what the benefit of expanding in IR was/is. Was that needed because there was no common definition for these intrinsics/nodes?
> 
> The ARM override says:
>       // Can't legalize reductions with soft floats, and NoNan will create
>       // fminimum which we do not know how to lower.
>       return TLI->useSoftFloat() || !TLI->getSubtarget()->hasFPRegs() ||
>              !II->getFastMathFlags().noNaNs();
> 
> So at the least I should update the comment. Leave the TLI checks but remove the FMF check?
That's right, you can drop the noNaNs check now (there should be a similar one in AArch64). This is intended to never use the IR expansion unless needed to avoid SDAG assertions. Those will be gone for the nnan case now.


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

https://reviews.llvm.org/D87391