[PATCH] D104854: Introduce intrinsic llvm.isnan

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
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:
> 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.
Well, if the **must never raise exceptions** is an IEEE-754 requirement (i.e. as noted in 5.7.2), I think it is reasonable that operations on types that do not conform to IEEE-754 are not bound by it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854