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