[PATCH] D137655: Expand fminimum and fmaximum into a pair of selects

Thu Nov 10 01:40:03 PST 2022

gflegar added a comment.

In D137655#3917621 <https://reviews.llvm.org/D137655#3917621>, @nikic wrote:

> In D137655#3917468 <https://reviews.llvm.org/D137655#3917468>, @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.
>>
>> Unfortunately, I don't have the bandwidth to chase this rabbit hole further, especially since our use case is insensitive to what happens for -0 and +0. I can add a TODO comment to fix this. Though I would still argue for submitting this, as it is correct modulo -0/+0, which is far preferable to the current state where we have a silent failure (and produce invalid code) for the backends that attempt to expand the op (like in NVPTX).
>
> That sounds like an unrelated bug. If an operation is Expand, but we don't support expanding it, shouldn't that result in an isel failure?

Yes, there is an orthogonal bug where this is a silent, and not a real failure. However, even if that bug is fixed, we would still fail (just earlier), which is fixed by this change.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137655