[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added a comment.

In D104854#2932444 <https://reviews.llvm.org/D104854#2932444>, @nemanjai wrote:

> Rather than reverting this commit again, I pushed 62fe3dcf98d1 <https://reviews.llvm.org/rG62fe3dcf98d17ea027492fd723dbb9b6dc295761> to use the same expansion as before (using unordered comparison) for `ppc_fp128`.

Thank you very much for fixing that!

> I am not sure if there are other types that suffer from the same problem (perhaps the Intel 80-bit long double).

Intel `fp80` is also non-IEEE type but it got custom lowering in this patch. There is little chance for such type to work properly without custom lowering.

I am working on patch that would add custom lowering of `llvm.isnan` to PowerPC.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6964
+    return DAG.getSetCC(DL, ResultVT, Op, DAG.getConstantFP(0.0, DL, OperandVT),
+                        ISD::SETUO);
+
----------------
nemanjai wrote:
> sepavloff wrote:
> > 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.
> >  
> Out of curiosity, why was this added when you recognized that it results in worse code? This is certainly part of the reason for the regression for `ppc_fp128`.
> 
> It would appear that before this patch, we would emit a comparison for all types that are not IEEE FP types (such as `ppc_fp128`). Those semantics do not seem to have carried over.
> Out of curiosity, why was this added when you recognized that it results in worse code? This is certainly part of the reason for the regression for ppc_fp128.

It is my mistake. After experiments I forgot to remove this change. I am sorry.

For x86 and AArch64 I used modified `test-suite`, with changes from D106804. Without proper tests it is hard to reveal why one intrinsic starts to fail.

> It would appear that before this patch, we would emit a comparison for all types that are not IEEE FP types (such as ppc_fp128). Those semantics do not seem to have carried over.

The previous behavior is not correct in non-default FP environment. Unordered comparison raises Invalid exception if an operand is signaling NaN. On the other hand, `isnan` must never raise exceptions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854