[PATCH] D104854: Introduce intrinsic llvm.isnan

Serge Pavlov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 10 09:18:09 PDT 2021


sepavloff 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);
+
----------------
nemanjai wrote:
> 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.
C standard defines macro `isnan`, of which the recent draft (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2596.pdf, F.3p6) states:

    The C classification macros fpclassify, iscanonical, isfinite, isinf, isnan, isnormal,
    issignaling, issubnormal, and iszero provide the IEC 60559 operations indicated in the table above 
    provided their arguments are in the format of their semantic type. Then these macros
    raise no floating-point exceptions, even if an argument is a signaling NaN.

This statement is not restricted to IEEE-compatible types, so any floating point type must behave according to this statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854



More information about the llvm-commits mailing list