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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
