[PATCH] D93179: [X86] Convert fmin/fmax _mm_reduce_* intrinsics to emit llvm.reduction intrinsics (PR47506)

[Pengfei Wang via Phabricator via cfe-commits]

pengfei added inline comments.


================
Comment at: clang/lib/Headers/avx512fintrin.h:9305
+ * 1. The elements are reassociable when using fadd/fmul intrinsics;
+ * 2. There's no nan and signed zero in the elements when using fmin/max
+ intrinsics;
----------------
spatel wrote:
> pengfei wrote:
> > spatel wrote:
> > > If I understand correctly, there's nothing preventing -0.0 or NaN values in these ops. But if either of those are present, then the result is potentially indeterminate.
> > > 
> > > For the LLVM intrinsic, we have this text in LangRef:
> > > "The result will always be a number unless all elements of the vector are NaN. For a vector with minimum element magnitude 0.0 and containing both +0.0 and -0.0 elements, the sign of the result is unspecified."
> > Thanks @spatel for the information. I checked that the LLVM intrinsic does ignore the sign of zeros. https://godbolt.org/z/a9Yj8a. So we can remove the no signed zero assumption.
> > But X86 fmin/fmax instructions have difference with the LLVM intrinsic, i.e. the result might be NaN even there's only one NaN in elements.
> > Besides, the LangRef also says "If the intrinsic call has the nnan fast-math flag, then the operation can assume that NaNs are not present in the input vector."
> > So we still need the no nan assumption here.
> I understand the behavior that we want to specify for this API, but we are not stating it clearly for the user. I think we should do that. How about:
> 
> ```
>  * For floating-point intrinsics:
>  * 1. When using fadd/fmul intrinsics, the order of operations within the vector is unspecified (associative math).
>  * 2. When using fmin/fmax intrinsics, NaN or -0.0 elements within the vector produce unspecified results. 
> ```
Great! It's much clear now. Thanks @spatel !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93179