[PATCH] D104854: Introduce intrinsic llvm.isnan

thopre added a comment. Ah fair enough. Thanks. 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://

[PATCH] D104854: Introduce intrinsic llvm.isnan

kpn added a comment. In D104854#2959680 , @thopre wrote: > In D104854#2957735 , @kpn wrote: > >> In D104854#2957490 , @lebedev.ri >> wrote: >> >>> In D104854#2957471

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added a comment. I posted the RFC: https://lists.llvm.org/pipermail/llvm-dev/2021-August/152257.html Depending on the feedback I'll revert the check or modify the implementation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104854/new/

[PATCH] D104854: Introduce intrinsic llvm.isnan

lebedev.ri added a subscriber: hans. lebedev.ri added a comment. In D104854#2959680 , @thopre wrote: > In D104854#2957735 , @kpn wrote: > >> In D104854#2957490 , @lebedev.

[PATCH] D104854: Introduce intrinsic llvm.isnan

thopre added a comment. In D104854#2957735 , @kpn wrote: > In D104854#2957490 , @lebedev.ri > wrote: > >> In D104854#2957471 , @sepavloff >> wrote: >> >>> In D104854#295

[PATCH] D104854: Introduce intrinsic llvm.isnan

kpn added a comment. In D104854#2957490 , @lebedev.ri wrote: > In D104854#2957471 , @sepavloff > wrote: > >> In D104854#2957423 , @spatel wrote: >> >>> Is it intentional

[PATCH] D104854: Introduce intrinsic llvm.isnan

spatel added a comment. In D104854#2957582 , @lebedev.ri wrote: > In D104854#2957529 , @spatel wrote: > >> In D104854#2957471 , @sepavloff >> wrote: >> >>> In D104854#29

[PATCH] D104854: Introduce intrinsic llvm.isnan

lebedev.ri added a comment. In D104854#2957529 , @spatel wrote: > In D104854#2957471 , @sepavloff > wrote: > >> In D104854#2957423 , @spatel wrote: >> >>> Is it intention

[PATCH] D104854: Introduce intrinsic llvm.isnan

xbolva00 added a comment. In D104854#2957529 , @spatel wrote: > In D104854#2957471 , @sepavloff > wrote: > >> In D104854#2957423 , @spatel wrote: >> >>> Is it intentional

[PATCH] D104854: Introduce intrinsic llvm.isnan

spatel added a comment. In D104854#2957471 , @sepavloff wrote: > In D104854#2957423 , @spatel wrote: > >> Is it intentional that we are not canonicalizing the intrinsic call back to >> `fcmp uno` in the default F

[PATCH] D104854: Introduce intrinsic llvm.isnan

lebedev.ri added a comment. In D104854#2957471 , @sepavloff wrote: > In D104854#2957423 , @spatel wrote: > >> Is it intentional that we are not canonicalizing the intrinsic call back to >> `fcmp uno` in the defau

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added a comment. In D104854#2957423 , @spatel wrote: > Is it intentional that we are not canonicalizing the intrinsic call back to > `fcmp uno` in the default FP environment? It is lowered to unordered comparison by default. Changing `llvm.isn

[PATCH] D104854: Introduce intrinsic llvm.isnan

spatel added a comment. Is it intentional that we are not canonicalizing the intrinsic call back to `fcmp uno` in the default FP environment? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104854/new/ https://reviews.llvm.org/D104854 _

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added a comment. Patch D108037 must make lowering of `llvm.isnan` more close to the code produced by `__builtin_isnan` earlier. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104854/new/ https://reviews.

[PATCH] D104854: Introduce intrinsic llvm.isnan

sivachandra added inline comments. Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22193 +DAG.getConstant(0x45, DL, MVT::i8)); + + return DAG.getSetCC(DL, ResultVT, Extract, DAG.getConstant(1, DL, MVT::i8), While I do not understand t

[PATCH] D104854: Introduce intrinsic llvm.isnan

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: > > ne

[PATCH] D104854: Introduce intrinsic llvm.isnan

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: > > sep

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added a comment. In D104854#2932444 , @nemanjai wrote: > Rather than reverting this commit again, I pushed 62fe3dcf98d1 > to use > the same expansion as before (using unorde

[PATCH] D104854: Introduce intrinsic llvm.isnan

nemanjai added a comment. Rather than reverting this commit again, I pushed 62fe3dcf98d1 to use the same expansion as before (using unordered comparison) for `ppc_fp128`. I am not sure if there are other types that suffer fro

[PATCH] D104854: Introduce intrinsic llvm.isnan

nemanjai added a comment. In D104854#2932186 , @nemanjai wrote: > This appears to have caused some failures on PPC buildbots. For example: > https://lab.llvm.org/buildbot/#/builders/105/builds/13446 > We are investigating this. Can you please pull this t

[PATCH] D104854: Introduce intrinsic llvm.isnan

nemanjai added a comment. This appears to have caused some failures on PPC buildbots. For example: https://lab.llvm.org/buildbot/#/builders/105/builds/13446 We are investigating this. Can you please pull this to bring the bots back to green until we track down the reason for the problem and can

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added a comment. Thanks! 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.or

[PATCH] D104854: Introduce intrinsic llvm.isnan

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM. (Since there have been a bunch of reviewers involved, please give a few days before you merge.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added inline comments. Comment at: llvm/test/Transforms/InstCombine/fpclass.ll:29 + ret <2 x i1> %t +} + RKSimon wrote: > You probably need some negative tests (no flags, ninf instead of nnan etc.)? Added few such tests. Repository: rG LLVM Github

[PATCH] D104854: Introduce intrinsic llvm.isnan

RKSimon added inline comments. Comment at: llvm/test/Transforms/InstCombine/fpclass.ll:29 + ret <2 x i1> %t +} + You probably need some negative tests (no flags, ninf instead of nnan etc.)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added inline comments. Comment at: llvm/test/CodeGen/X86/x86-fpclass.ll:173 + +define <1 x i1> @isnan_double_vec1(<1 x double> %x) { +; CHECK-32-LABEL: isnan_double_vec1: RKSimon wrote: > add nounwind to reduce cfi noise (other tests would benefit as we

[PATCH] D104854: Introduce intrinsic llvm.isnan

RKSimon added inline comments. Comment at: llvm/test/CodeGen/X86/x86-fpclass.ll:173 + +define <1 x i1> @isnan_double_vec1(<1 x double> %x) { +; CHECK-32-LABEL: isnan_double_vec1: add nounwind to reduce cfi noise (other tests would benefit as well)?

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added a comment. In D104854#2905430 , @efriedma wrote: > In D104854#2904826 , @sepavloff > wrote: > >> In D104854#2886328 , @efriedma >> wrote: >> The opt

[PATCH] D104854: Introduce intrinsic llvm.isnan

efriedma added a comment. In D104854#2904826 , @sepavloff wrote: > In D104854#2886328 , @efriedma > wrote: > >>> The options '-ffast-math' and '-fno-honor-nans' imply that FP operation >>> operands are never NaNs

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added inline comments. Comment at: llvm/docs/LangRef.rst:20983 + +These functions get properties of floating point values. + tschuett wrote: > Fixed, thank you. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff updated this revision to Diff 361718. sepavloff added a comment. Fixed documentation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104854/new/ https://reviews.llvm.org/D104854 Files: clang/lib/CodeGen/CGBuiltin.cpp clang/test/CodeGen

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added a comment. In D104854#2886328 , @efriedma wrote: >> The options '-ffast-math' and '-fno-honor-nans' imply that FP operation >> operands are never NaNs. This assumption however should not be applied >> to the functions that check FP number

[PATCH] D104854: Introduce intrinsic llvm.isnan

tschuett added inline comments. Comment at: llvm/docs/LangRef.rst:20983 + +These functions get properties of floating point values. + Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104854/new/ https://reviews.llvm

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff updated this revision to Diff 361707. sepavloff added a comment. Updated patch - Rebased, - Applied small enhancement to integer implementation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104854/new/ https://reviews.llvm.org/D104854

[PATCH] D104854: Introduce intrinsic llvm.isnan

efriedma added a comment. > The options '-ffast-math' and '-fno-honor-nans' imply that FP operation > operands are never NaNs. This assumption however should not be applied > to the functions that check FP number properties, including 'isnan'. If > such function returns expected result instead of

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff updated this revision to Diff 357857. sepavloff added a comment. Rebased Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104854/new/ https://reviews.llvm.org/D104854 Files: clang/lib/CodeGen/CGBuiltin.cpp clang/test/CodeGen/X86/strictf

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff updated this revision to Diff 355614. sepavloff added a comment. Missed optimization in X86 codegen proposed by Craig Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104854/new/ https://reviews.llvm.org/D104854 Files: clang/lib/CodeGen/C

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added a comment. In D104854#2841505 , @efriedma wrote: > I'd like to see some test coverage for all the floating-point types (half, > bfloat16, ppc_fp128). Tests for half are added to aarch64-fpclass.ll, new file was created to test ppc_fp128

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff updated this revision to Diff 355539. sepavloff added a comment. Herald added subscribers: kbarton, nemanjai. Addressed reviewer's notes - Math flags are set when ISNAN node is transformed, - They are set in calls to getNode, - WidenVecOp_ISNAN is made similar to WidenVecOp_SETCC, - Bui

[PATCH] D104854: Introduce intrinsic llvm.isnan

efriedma added a comment. A few of the AArch64 sequences don't look ideal, but that's not the fault of your patch. I'd like to see some test coverage for all the floating-point types (half, bfloat16, ppc_fp128). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D104854: Introduce intrinsic llvm.isnan

craig.topper added inline comments. Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22039 + // Move FPSW to AX. + SDValue FPSW = DAG.getCopyToReg(DAG.getEntryNode(), DL, X86::FPSW, Test, + SDValue()); The code you copied thi

[PATCH] D104854: Introduce intrinsic llvm.isnan

craig.topper added inline comments. Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:662 + EVT NewResultVT = TLI.getTypeToTransformTo(*DAG.getContext(), ResultVT); + return DAG.getNode(N->getOpcode(), DL, NewResultVT, N->getOperand(0)); +} Don

[PATCH] D104854: Introduce intrinsic llvm.isnan

thopre added a comment. LGTM (besides comment fix) but I'm not too familiar with the vector side of things Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6967 + // NaN has all exp bits set and a non zero significand. Therefore: + // isnan(V) == ((exp mask - (ab

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added a comment. In D104854#2838754 , @craig.topper wrote: > Doesn't gcc also fold isnan to false under fast math? If we diverge here that > means your code would only work correctly with clang. GCC does the same transformation, ICC and MSVC

[PATCH] D104854: Introduce intrinsic llvm.isnan

craig.topper added a comment. Doesn't gcc also fold isnan to false under fast math? If we diverge here that means your code would only work correctly with clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104854/new/ https://reviews.llvm.org/D1

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added a comment. In D104854#2838659 , @jdoerfert wrote: > In D104854#2838495 , @sepavloff > wrote: > >> In D104854#2838468 , @thopre wrote: >> >>> Are you plann

[PATCH] D104854: Introduce intrinsic llvm.isnan

jdoerfert added a comment. In D104854#2838495 , @sepavloff wrote: > In D104854#2838468 , @thopre wrote: > >> Are you planning to do this for the other FP test builtin (isinf, isfinite, >> isinf_sign, isnormal)? >

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff added a comment. In D104854#2838468 , @thopre wrote: > Are you planning to do this for the other FP test builtin (isinf, isfinite, > isinf_sign, isnormal)? Yes, they have similar problems. If someone would like to implement them it would be n

[PATCH] D104854: Introduce intrinsic llvm.isnan

thopre added a comment. Are you planning to do this for the other FP test builtin (isinf, isfinite, isinf_sign, isnormal)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104854/new/ https://reviews.llvm.org/D104854

[PATCH] D104854: Introduce intrinsic llvm.isnan

sepavloff created this revision. sepavloff added reviewers: efriedma, kpn, thopre, jonpa, cameron.mcinally, RKSimon, craig.topper. Herald added subscribers: jdoerfert, pengfei, hiraditya. sepavloff requested review of this revision. Herald added projects: clang, LLVM. Herald added a subscriber: cf