[PATCH] D151482: [LV] Add support for minimum/maximum intrinsics

Thu Jun 1 10:10:30 PDT 2023

anna added a comment.

In D151482#4386222 <https://reviews.llvm.org/D151482#4386222>, @skatkov wrote:

> In D151482#4385109 <https://reviews.llvm.org/D151482#4385109>, @anna wrote:
>
>> Looking at X86 lowering for the vector reduce intrinsics, we do not handle signed zeroes. So, we will need to introduce new intrinsics as `llvm.vector.reduce.fminimum` and `llvm.vector.reduce.fmaximum`, which is similar to existing vector reduce intrinsics but handles signed zeroes and NaN.
>
> My understanding is as follows:
> You need to inroduce new reduce intrinsic for fminium and fmaximum as you wrote.
> In ExpandReductionsPass replace this intrinsic with reduction where you delegates each comparison to existing fminimum/maximum intrinsics. You just need to propagate nnan and nsz flags to that inrinisics.
>
> With that you will have a correct reduction supporting both NaNs and signed zeros.

This is a great idea to use the shuffle reductions as the generic lowering. Also, IIUC, propagating the `nnan` and `nsz` is an optimization, not necessary for correctness since fminimum and fmaximum supports NaNs and signed zeroes.

There is one comment in the shuffle reductions stating that we need `nnan` to perform a shuffle reduction (under the `vector_reduce_fmax` and `vector_reduce_fmin` categories), but I don't see anything in LangRef stating that the shuffles themselves do not support NaNs. So, we do not need this extra check of `nnan` flag when adding the fminimum and fmaximum intrinsic support here. In fact, I think if anyone's interested, one can even remove this restriction from `vector_reduce_fmax/vector_reduce_fmin` themselves once we add support for fminnum/fmaxnum in `createMinMaxOp`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151482