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

[Dave Green via Phabricator via llvm-commits]

dmgreen 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
----------------
nikic wrote:
> 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.
> It's not clear to me what the benefit of expanding in IR was/is.

I agree. I think a lot of it was legacy, and expanding during ISel seems like a better way forward if we can make it work.


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

https://reviews.llvm.org/D87391