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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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
----------------
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?


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

https://reviews.llvm.org/D87391