[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D104854#2905430 <https://reviews.llvm.org/D104854#2905430>, @efriedma wrote:

> In D104854#2904826 <https://reviews.llvm.org/D104854#2904826>, @sepavloff wrote:
>
>> In D104854#2886328 <https://reviews.llvm.org/D104854#2886328>, @efriedma wrote:
>>
>>>> The options '-ffast-math' and '-fno-honor-nans' imply that FP operation
>>>> operands are never NaNs. This assumption however should not be applied
>>>> to the functions that check FP number properties, including 'isnan'. If
>>>> such function returns expected result instead of actually making
>>>> checks, it becomes useless in many cases.
>>>
>>> This doesn't work the way you want it to, at least given the way nnan/ninf are currently defined in LangRef.  It's possible to end up in a situation where `isnan(x) == isnan(x)` evaluates to false at runtime.  It doesn't matter how you compute isnan; the problem is that the input is poisoned.
>>>
>>> I think the right solution to this sort of issue is to insert a "freeze" in the LLVM IR, or something like that.  Not sure how we'd expect users to write this in C.  Suggestions welcome.
>>
>> According to the documentation, nnan/ninf may be applied to `fneg`, `fadd`, `fsub`, `fmul`, `fdiv`, `frem`, `fcmp`, `phi`, `select` and `call`. We can ignore this flag for calls of isnan and similar functions. Of course, if conditions of using `-ffast-math` are broken, we have undefined behavior and `isnan(x) != isnan(x)` becomes possible, like in this code:
>
> Right... so how can you produce a NaN in these circumstances?  You could load one from memory, I guess?

Yes, they come from structures in memory. I think they can also come from function arguments, if some source files are compiled with option `-ffast-math` and some without.

> It would probably be a good idea to have an instcombine that combines away isnan on a value produced by an operation marked nnan, so we don't confuse people reading assembly into assuming isnan is actually reliable in that context.

Added such transformation (file llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp, test in llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854