[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

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:

  %c = fadd %a, nan
  %r = call llvm.isnan.f32(%c) 

Similarly, it is legitimate to optimize `isnan` in the code:

  %c = fadd %a, %b
  %r = call llvm.isnan.f32(%c)

In this case the result of `fadd` cannot be NaN, otherwise contract of `-ffast-math` is broken. So `isnan` in this case may be optimized to `false`.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6964
+    return DAG.getSetCC(DL, ResultVT, Op, DAG.getConstantFP(0.0, DL, OperandVT),
+                        ISD::SETUO);
+
----------------
efriedma wrote:
> Maybe we want to consider falling back to the integer path if SETCC isn't legal for the given operand type?  We could do that as a followup, though.
It makes sense, it could be beneficial for targets that have limited set of floating point comparisons. However straightforward check like:

    if (Flags.hasNoFPExcept() && isOperationLegalOrCustom(ISD::SETCC, OperandVT))

results in worse code, mainly for vector types. It should be more complex check.
 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6984
+  return DAG.getSetCC(DL, ResultVT, Sub, DAG.getConstant(0, DL, IntVT),
+                      ISD::SETNE);
+}
----------------
efriedma wrote:
> Instead of emitting `ExpMaskV - AbsV != 0`, can we just emit `ExpMaskV != AbsV`?
> Instead of emitting ExpMaskV - AbsV != 0, can we just emit ExpMaskV != AbsV?

Implemented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854