[PATCH] D137655: Expand fminimum/fmaximum into fminnum/fmaxnum + NaN check

Thu Nov 10 10:56:55 PST 2022

arsenm added a comment.

In D137655#3919749 <https://reviews.llvm.org/D137655#3919749>, @gflegar wrote:

> In D137655#3919674 <https://reviews.llvm.org/D137655#3919674>, @arsenm wrote:
>
>> In D137655#3918964 <https://reviews.llvm.org/D137655#3918964>, @gflegar wrote:
>>
>>> In D137655#3917359 <https://reviews.llvm.org/D137655#3917359>, @nikic wrote:
>>>
>>>> I don't think this is right either. LLVM defines minnum according to the old semantics, which don't specify an order between zeroes. We'd need a separate ISD opcode for minnum according to 2018 semantics.
>>>
>>> Interestingly though, for the NVPTX backend this will end up producing the correct code, since minnum is lowered to the min/max PTX instructions, which defines the 2018 semantics. (I do agree though that the intermediate code is not correct.)
>>
>> It's not OK to have wrong intermediate code. We do have the "new" semantic opcodes already in FMINIMUM/FMAXIMUM
>
> It's even less OK to fail altogether (which is what is happening without this patch).

Hard disagree

> And we're not talking about the new semantics for FMINIMUM/FMAXIMUM, but for the new semantics of FMINNUM/FMAXNUM (the +/-0 handling changed).

The 2019 final spec does not have minnum or maxnum; they were removed and replaced with minimum and maximum which have specified signed zero behavior. Unless there was a draft revision I missed, there was never a defined minnum with specified -0 behavior.  It would be helpful to define minnum/maxnum variants with specified -0 ordered less than +0

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137655