[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

alexbatashev marked 6 inline comments as done. alexbatashev added a comment. @Ka-Ka done CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62580/new/ https://reviews.llvm.org/D62580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

Ka-Ka accepted this revision. Ka-Ka added a comment. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62580/new/ https://reviews.llvm.org/D62580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

Ka-Ka added inline comments. Comment at: clang/test/CodeGen/builtins-x86.c:127-128 - tmp_V2LLi = __builtin_ia32_undef128(); - tmp_V4LLi = __builtin_ia32_undef256(); + tmp_V2LLi = (V2LLi)__builtin_ia32_undef128(); + tmp_V4LLi = (V4LLi)__builtin_ia32_undef256();

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

Anastasia accepted this revision. Anastasia added a comment. LGTM! Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62580/new/ https://reviews.llvm.org/D62580 ___ cfe-commits mailing list

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

alexbatashev marked an inline comment as done. alexbatashev added a comment. @Ka-Ka good point. Thank you. @Anastasia Would such tests be ok with you? @erichkeane Thank you very much. I think I don't have permissions to commit changes and will need someone's help. Repository: rG LLVM Github

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

Anastasia added a comment. Do you think it's possible to add a test? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62580/new/ https://reviews.llvm.org/D62580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

Ka-Ka added a comment. What about a testcase? It shouldn't be hard to add a small testcase that demonstrate that the changed builtins now work when compiling OpenCL C code for x86. I don't think you have to add all changed builtins to the testcase (but a few to demonstrate the change).

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Conditional approval, commit with the comment fixed. Let me know if you need me to commit it for you. Comment at: clang/include/clang/Basic/Builtins.def:56 // N

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

alexbatashev added a comment. In D62580#1521234 , @erichkeane wrote: > 'O' is an interesting choice. Any real justification for it, or just "what > was available"? It definitely needs to be documented in the top of > Builtins.def however. O is for

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

craig.topper added a comment. In D62580#1521277 , @erichkeane wrote: > A different (perhaps silly) question is why 'W' isn't sufficient? It > represents int64_t, which I wonder if is sufficient. I had asked in a separate conversation not to change the

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

craig.topper added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:9308 case 'L': assert(!IsSpecial && "Can't use 'L' with 'W', 'N' or 'Z' modifiers"); assert(HowLong <= 2 && "Can't have modifier"); O should be mentioned here

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

erichkeane added a comment. A different (perhaps silly) question is why 'W' isn't sufficient? It represents int64_t, which I wonder if is sufficient. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62580/new/ https://reviews.llvm.org/D62580

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

erichkeane added a comment. 'O' is an interesting choice. Any real justification for it, or just "what was available"? It definitely needs to be documented in the top of Builtins.def however. Comment at: clang/lib/AST/ASTContext.cpp:9362 +case 'O': +