[PATCH] D104854: Introduce intrinsic llvm.isnan

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

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`. I am not sure if there are other types that suffer from the same problem (perhaps the Intel 80-bit long double).



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6964
+    return DAG.getSetCC(DL, ResultVT, Op, DAG.getConstantFP(0.0, DL, OperandVT),
+                        ISD::SETUO);
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854