[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

nastafev added a comment. Thanks, I agree with @andrew.w.kaylor and his interpretation. I was trying to convey the message that the programmer operating with intrinsics relies on the semantics they carry because there's no other way to express that semantics. Re-optimizing what's already optimiz

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

andrew.w.kaylor added a comment. Agreed. Reverting this patch wouldn't move us forward on constrained FP handling. What I'm saying (and what I think @nastafev is saying) is that this patch is taking a built-in that allows the user to express specific signalling behavior and throwing away that

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

efriedma added a comment. Yes, in constrained-fp mode we might need intrinsics, at least short-term. I assume you'll probably add target-independent constrained masked fp vector operations at some point, but that's probably not a priority. But that still leaves two problems. One, clang doesn

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

andrew.w.kaylor added a comment. In https://reviews.llvm.org/D45616#1290897, @nastafev wrote: > > can trigger arbitrary floating-point exceptions anywhere in your code > > I believe this statement reflects the current state of many compilers on the > market, I guess I just don't see the reason

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

nastafev added a comment. > can trigger arbitrary floating-point exceptions anywhere in your code I believe this statement reflects the current state of many compilers on the market, I guess I just don't see the reason why making things worse. It seems the original intent of the commit was to

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

efriedma added a comment. > It seems you were well aware that you are changing the semantics of FP > operation here by ignoring the signaling/quiet portion of the immediate. There's ongoing work to support code that accesses the floating point environment (see http://llvm.org/docs/LangRef.html

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

nastafev added a comment. Hello. It seems you were well aware that you are changing the semantics of FP operation here by ignoring the signaling/quiet portion of the immediate. But what shall the user do now? There was no way to force quiet FP comparison behavior in C language, so intrinsics an

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

This revision was automatically updated to reflect the committed changes. Closed by commit rC335339: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR (authored by GBuella, committed by ). Changed prior to commit: https://reviews.llvm.org/D45616?vs=152060&id=152456#toc Reposito

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

craig.topper accepted this revision. craig.topper added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D45616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/li

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella added a comment. I was overzealous with the intrinsics, I lower really only the packed comparison now. https://reviews.llvm.org/D45616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cf

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella updated this revision to Diff 152060. GBuella edited the summary of this revision. https://reviews.llvm.org/D45616 Files: lib/CodeGen/CGBuiltin.cpp test/CodeGen/avx-builtins.c test/CodeGen/avx-cmp-builtins.c test/CodeGen/avx2-builtins.c test/CodeGen/avx512f-builtins.c test/Cod

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

craig.topper requested changes to this revision. craig.topper added inline comments. This revision now requires changes to proceed. Comment at: test/CodeGen/avx-builtins.c:241 // CHECK-LABEL: test_mm_cmp_sd - // CHECK: call <2 x double> @llvm.x86.sse2.cmp.sd(<2 x double> %{{.

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella updated this revision to Diff 151884. GBuella added a comment. Added `__builtin_ia32_cmpsd_mask` & `__builtin_ia32_cmpss_mask`. https://reviews.llvm.org/D45616 Files: lib/CodeGen/CGBuiltin.cpp test/CodeGen/avx-builtins.c test/CodeGen/avx-cmp-builtins.c test/CodeGen/avx2-builtins

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

craig.topper added a comment. We should probably just leave them around. Leave a NOTE so they don't get deleted. https://reviews.llvm.org/D45616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella updated this revision to Diff 151710. https://reviews.llvm.org/D45616 Files: lib/CodeGen/CGBuiltin.cpp test/CodeGen/avx-builtins.c test/CodeGen/avx-cmp-builtins.c test/CodeGen/avx2-builtins.c test/CodeGen/avx512f-builtins.c test/CodeGen/avx512vl-builtins.c Index: test/CodeGen

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella added a comment. The question still left is, should we remove, auto upgrade the LLVM intrinsics not used anymore, or keep them around for when the signal behaviour is going to matter? https://reviews.llvm.org/D45616 ___ cfe-commits mailing

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

craig.topper accepted this revision. craig.topper added a comment. This revision is now accepted and ready to land. LGTM with that one comment. Comment at: lib/CodeGen/CGBuiltin.cpp:10070 + +assert(CC < 0x20 && "condition code should be validated by sema checking"); + -

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

spatel added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:10107-10112 +case 0x0b: // FALSE_OQ +case 0x1b: // FALSE_OS + return llvm::Constant::getNullValue(ConvertType(E->getType())); +case 0x0f: // TRUE_UQ +case 0x1f: // TRUE_US + return llvm:

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:10107-10112 +case 0x0b: // FALSE_OQ +case 0x1b: // FALSE_OS + return llvm::Constant::getNullValue(ConvertType(E->getType())); +case 0x0f: // TRUE_UQ +case 0x1f: // TRUE_US + return llvm

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

spatel added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:10107-10112 +case 0x0b: // FALSE_OQ +case 0x1b: // FALSE_OS + return llvm::Constant::getNullValue(ConvertType(E->getType())); +case 0x0f: // TRUE_UQ +case 0x1f: // TRUE_US + return llvm:

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella updated this revision to Diff 151203. GBuella edited the summary of this revision. GBuella added a comment. Ignoring signaling behviour - and rounding mode with it. Also lowering `__builtin_ia32_cmpsd` and `__builtin_ia32_cmpss`. https://reviews.llvm.org/D45616 Files: lib/CodeGen/CGBu

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:10090-10100 +// _CMP_TRUE_UQ, _CMP_TRUE_US produce -1,-1... vector +// on any input and _CMP_FALSE_OQ, _CMP_FALSE_OS produce 0, 0... +if (CC == 0xf || CC == 0xb || CC == 0x1b || CC == 0x1f) { +

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

spatel added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:10090-10100 +// _CMP_TRUE_UQ, _CMP_TRUE_US produce -1,-1... vector +// on any input and _CMP_FALSE_OQ, _CMP_FALSE_OS produce 0, 0... +if (CC == 0xf || CC == 0xb || CC == 0x1b || CC == 0x1f) { +

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

craig.topper added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:10071 + // is _MM_FROUND_CUR_DIRECTION + if (cast(Ops[4])->getZExtValue() != 4) +UsesNonDefaultRounding = true; GBuella wrote: > efriedma wrote: > > Given we're ignoring f

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:10071 + // is _MM_FROUND_CUR_DIRECTION + if (cast(Ops[4])->getZExtValue() != 4) +UsesNonDefaultRounding = true; efriedma wrote: > Given we're ignoring floating-point exceptions

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

efriedma added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:10071 + // is _MM_FROUND_CUR_DIRECTION + if (cast(Ops[4])->getZExtValue() != 4) +UsesNonDefaultRounding = true; Given we're ignoring floating-point exceptions, we should also

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella added a comment. Ping @efriedma https://reviews.llvm.org/D45616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella updated this revision to Diff 150767. GBuella added a comment. I altered the code, to ignore the the signaling behaviour, as suggested. Also, it handles more vector cmp builtins now. https://reviews.llvm.org/D45616 Files: lib/CodeGen/CGBuiltin.cpp test/CodeGen/avx-builtins.c test/

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

efriedma added a subscriber: andrew.w.kaylor. efriedma added a comment. I don't see any reason to exactly match the constant specified by the user, as long as the result is semantically equivalent. Once we have llvm.experimental.constrained.fcmp, this code should be updated to emit it; that wil

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella added a comment. Ping @efriedma Repository: rC Clang https://reviews.llvm.org/D45616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella requested review of this revision. GBuella added a comment. Oops, accepted this by accident. Repository: rC Clang https://reviews.llvm.org/D45616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailma

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella accepted this revision. GBuella added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D45616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella added a comment. In https://reviews.llvm.org/D45616#1093514, @efriedma wrote: > There is no difference between "signalling" and "non-signalling" unless > you're using "#pragma STDC FENV_ACCESS", which is currently not supported. > Presumably the work to implement that will include some

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

efriedma added a comment. There is no difference between "signalling" and "non-signalling" unless you're using "#pragma STDC FENV_ACCESS", which is currently not supported. Presumably the work to implement that will include some LLVM IR intrinsic which can encode the difference, but for now we

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella added a comment. In https://reviews.llvm.org/D45616#1067492, @efriedma wrote: > > The fcmp opcode has no defined behavior with NaN operands in the > > comparisions handled in this patch. > > Could you describe the problem here in a bit more detail? As far as I know, > the LLVM IR fcmp

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

efriedma added a comment. > The fcmp opcode has no defined behavior with NaN operands in the comparisions > handled in this patch. Could you describe the problem here in a bit more detail? As far as I know, the LLVM IR fcmp should return the same result as the x86 CMPPD, even without fast-mat

[PATCH] D45616: [X86] Lower _mm[256|512]_cmp[.]_mask intrinsics to native llvm IR

GBuella created this revision. GBuella added a reviewer: craig.topper. Herald added a subscriber: cfe-commits. The fcmp opcode has no defined behavior with NaN operands in the comparisions handled in this patch. Thus, these intrinsics can only be safe lowered to fcmp opcodes when fast-math is enab