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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

The LangRef change looks good to me. I've pointed out one place where the SDAG expansion doesn't seem quite compatible with it.



================
Comment at: llvm/lib/CodeGen/ExpandReductions.cpp:163
+              cast<FixedVectorType>(Vec->getType())->getNumElements()) ||
+          !FMF.isFast())
         continue;
----------------
Would it be sufficient to only check nnan here, or does the expansion rely on something more?


================
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
----------------
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 :/


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

https://reviews.llvm.org/D87391