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


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

https://reviews.llvm.org/D87391