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
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
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
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
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
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
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
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=152456#toc
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
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
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
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>
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
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
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:
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
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");
+
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
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
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
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:
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) {
+
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) {
+
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
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
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
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
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
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
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
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
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
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
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
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
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
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
37 matches
Mail list logo
