[PATCH] D79279: Add overloaded versions of builtin mem* functions

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. I think the documentation isn't quite right yet, but otherwise I think I'm happy. (With a couple of code change suggestions.) In D79279#2240487

[PATCH] D79279: Add overloaded versions of builtin mem* functions

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I went through all the comments here, plus looked at the code myself. I believe all of the comments by other reviewers have been fixed/answered acceptably. I don't have any

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2235085 , @rsmith wrote: > Thanks, I'm happy with this approach. > > If I understand correctly, the primary (perhaps sole) purpose of > `__builtin_memcpy_sized` is to provide a primitive from which an atomic > operation

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 288131. jfb marked 12 inline comments as done. jfb added a comment. Address Richard's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rsmith added a comment. Thanks, I'm happy with this approach. If I understand correctly, the primary (perhaps sole) purpose of `__builtin_memcpy_sized` is to provide a primitive from which an atomic operation can be produced. That being the case, I wonder if the name is emphasizing the wrong

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. Ping, I think I've addressed all comments here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 ___ cfe-commits mailing list

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. Actually I think any subsequent updates to tests can be done in a follow-up patch, since I'm not changing the status-quo on address space here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 285479. jfb added a comment. Fix a test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files: clang/docs/LanguageExtensions.rst

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 285414. jfb added a comment. Update overloading as discussed: on the original builtins, and separate the _sized variant, making its 4th parameter non-optional. If this looks good, I'll need to update codege for a few builtins (to handle volatile), as well as

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2201484 , @rsmith wrote: > I think it would be reasonable in general to guarantee that our `__builtin_` > functions have contracts at least as wide as the underlying C function, but > allow them to have extensions, and to

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rsmith added a comment. In D79279#2201136 , @rjmccall wrote: > In D79279#2200916 , @rsmith wrote: > >> In D79279#2197176 , @rjmccall wrote: >> >>> I thought part of the

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. In D79279#2200916 , @rsmith wrote: > In D79279#2197176 , @rjmccall wrote: > >> I thought part of the point of `__builtin_memcpy` was so that C library >> headers could do `#define

[PATCH] D79279: Add overloaded versions of builtin mem* functions

tstellar added a comment. In D79279#2200916 , @rsmith wrote: > In D79279#2197176 , @rjmccall wrote: > >> I thought part of the point of `__builtin_memcpy` was so that C library >> headers could do `#define

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rsmith added a comment. In D79279#2197176 , @rjmccall wrote: > I thought part of the point of `__builtin_memcpy` was so that C library > headers could do `#define memcpy(x, y, z) __builtin_memcpy(x, y, z)`. If so, > the conformance issue touches

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 283406. jfb marked 5 inline comments as done. jfb added a comment. Remove restrict, update docs, call isCompleteType Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2197118 , @rsmith wrote: > Two observations that are new to me in this review: > > 1. We already treat all builtins as being overloaded on address space. > 2. The revised patch treats `__builtin_mem*_overloaded` as being

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2446 +order in which they occur (and in which they are observable) can only be +guaranteed using appropriate fences around the function call. Element size must +therefore be a lock-free size for the target

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 283402. jfb marked an inline comment as done. jfb added a comment. Use 'access size' instead of 'element size'. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added inline comments. Comment at: compiler-rt/lib/ubsan/ubsan_handlers.cpp:659 + uptr PtrOrSize) { GET_REPORT_OPTIONS(true); + handleInvalidBuiltin(Data, Opts, PtrOrSize); vsk wrote: > It looks like

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 283400. jfb marked an inline comment as done. jfb added a comment. Add loop test requested by Vedant Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 283390. jfb added a comment. Do UBSan change suggested by Vedant. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files: clang/docs/LanguageExtensions.rst

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. I thought part of the point of `__builtin_memcpy` was so that C library headers could do `#define memcpy(x, y, z) __builtin_memcpy(x, y, z)`. If so, the conformance issue touches `__builtin_memcpy` as well, not just calls to the library builtin. If that's not true,

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rsmith added a comment. Two observations that are new to me in this review: 1. We already treat all builtins as being overloaded on address space. 2. The revised patch treats `__builtin_mem*_overloaded` as being overloaded *only* on address space, volatility, and atomicity. (We've tuned the

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2446 +order in which they occur (and in which they are observable) can only be +guaranteed using appropriate fences around the function call. Element size must +therefore be a lock-free size for the

[PATCH] D79279: Add overloaded versions of builtin mem* functions

vsk added inline comments. Comment at: compiler-rt/lib/ubsan/ubsan_handlers.cpp:659 + uptr PtrOrSize) { GET_REPORT_OPTIONS(true); + handleInvalidBuiltin(Data, Opts, PtrOrSize); It looks like

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2195299 , @rjmccall wrote: > Patch looks basically okay to me, although I'll second Richard's concern that > we shouldn't absent-mindedly start producing overloaded `memcpy`s for > ordinary `__builtin_memcpy`. Yeah I

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 283121. jfb marked 2 inline comments as done. jfb added a comment. Update docs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files: clang/docs/LanguageExtensions.rst

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. Patch looks basically okay to me, although I'll second Richard's concern that we shouldn't absent-mindedly start producing overloaded `memcpy`s for ordinary `__builtin_memcpy`. Comment at: clang/docs/LanguageExtensions.rst:2446 +order in which they

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rsmith added a comment. Thanks. I'd like @rjmccall to approve too, but the design of these intrinsics and the Sema and constant evaluation parts seem fine. (We don't strictly need constant evaluation to abort on library UB, so I think not catching the misalignment case is OK.)

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added inline comments. Comment at: compiler-rt/test/ubsan/TestCases/Misc/builtins-ctz-clz.cpp:1 +// REQUIRES: arch=x86_64 +// Phab is confused I did a git rename of `compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp` and it thinks this is new, and I

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. Thanks for the detailed comments, I think I've addressed all of them! I also added UBSan support to check the builtin invocation. I think this patch is pretty much ready to go. A follow-up will need to add the support functions to compiler-rt (they're currently optional,

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 283078. jfb marked 6 inline comments as done. jfb added a comment. Herald added a project: Sanitizers. Herald added a subscriber: Sanitizers. Address Richard's comments, add UBSan support. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rsmith added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2430 + +These overloads support destinations and sources which are a mix of the +following qualifiers: Comment at: clang/docs/LanguageExtensions.rst:2454 +and might

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. This is almost ready I think! There are a few things still open, I'd love feedback on them. Comment at: clang/docs/LanguageExtensions.rst:2435-2437 +* ``__builtin_memcpy_overloaded(QUAL void *dst, QUAL const void *src, size_t byte_size, size_t

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 282281. jfb marked 9 inline comments as done. jfb added a comment. Address Richard's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rsmith added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2435-2437 +* ``__builtin_memcpy_overloaded(QUAL void *dst, QUAL const void *src, size_t byte_size, size_t byte_element_size = )`` +* ``__builtin_memmove_overloaded(QUAL void *dst, QUAL const void *src,

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8793 +BuiltinOp == Builtin::BI__builtin_memmove_overloaded || BuiltinOp == Builtin::BI__builtin_wmemmove; If we end up

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a subscriber: dneilson. jfb added a comment. I've addressed @rsmith @rjmccall suggestions (unless noted), thanks! An open question: as of 6e4aa1e48138182685431c76184dfc36e620aea2 @dneilson added an assertion on `CreateElementUnorderedAtomicMemCpy` to check that the pointer arguments

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 281087. jfb marked 15 inline comments as done. jfb added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rsmith added a comment. These new builtins should ideally support constant evaluation if possible. Comment at: clang/docs/LanguageExtensions.rst:2439-2440 +Clang provides versions of the following functions which are overloaded based on +the pointer parameter types: +

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. >>> Do you think it'd be useful to have different guarantees for different >>> operands? I guess it could come up, but it'd be a whole lot of extra >>> complexity that I can't imagine we'd ever support. >> >> You mean, if `element_size` is passed then you get different

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. In D79279#2170187 , @jfb wrote: > In D79279#2170157 , @rjmccall wrote: > > > I think the argument is treated as if it were 1 if not given. That's all > > that ordinary memcpy formally

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2170157 , @rjmccall wrote: > I think the argument is treated as if it were 1 if not given. That's all > that ordinary memcpy formally guarantees, which seems to work fine > (semantically, if not performance-wise) for

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. I think the argument is treated as if it were 1 if not given. That's all that ordinary memcpy formally guarantees, which seems to work fine (semantically, if not performance-wise) for pretty much everything today. I don't think you need any restrictions on element

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2170095 , @rjmccall wrote: > I don't think any of these should allow _Atomic unless we're going to give it > some sort of consistent atomic semantics (which is hard to imagine being > useful), and I think you should just

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. I don't think any of these should allow _Atomic unless we're going to give it some sort of consistent atomic semantics (which is hard to imagine being useful), and I think you should just take an extra argument of the minimum access width on all of them uniformly if

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. > My point is that this has nothing to do with the ordinary semantics of > `_Atomic`. You're basically just looking at the word "atomic" and saying > that, hey, a minimum access size is sortof related to atomicity. > > If you want this to be able to control the minimum

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. In D79279#2169522 , @jfb wrote: > In D79279#2168649 , @rjmccall wrote: > > > In D79279#2168533 , @jfb wrote: > > > > > In D79279#2168479

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 280136. jfb added a comment. Re-update Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files: clang/docs/LanguageExtensions.rst clang/include/clang/Basic/Builtins.def

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 280135. jfb added a comment. Improve documentation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files: clang/docs/LanguageExtensions.rst Index:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2168649 , @rjmccall wrote: > In D79279#2168533 , @jfb wrote: > > > In D79279#2168479 , @rjmccall > > wrote: > > > > > Is there a need for an

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. In D79279#2168533 , @jfb wrote: > In D79279#2168479 , @rjmccall wrote: > > > Is there a need for an atomic memcpy at all? Why is it useful to allow > > this operation to take on "atomic"

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added a comment. In D79279#2168479 , @rjmccall wrote: > Is there a need for an atomic memcpy at all? Why is it useful to allow this > operation to take on "atomic" semantics — which aren't actually atomic > because the loads and stores to elements

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. Is there a need for an atomic memcpy at all? Why is it useful to allow this operation to take on "atomic" semantics — which aren't actually atomic because the loads and stores to elements are torn — with hardcoded memory ordering and somewhat arbitrary rules about

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8917 + +def err_atomic_type_must_be_lock_free : Error<"_Atomic type must always be lock-free, %0 isn't">; + rjmccall wrote: > I don't know why you're adding a bunch of new

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 279984. jfb marked 7 inline comments as done. jfb added a comment. Address all but one of John's comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. You need to add user docs for these builtins. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8917 + +def err_atomic_type_must_be_lock_free : Error<"_Atomic type must always be lock-free, %0 isn't">; + I don't know why

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 279896. jfb added a comment. Follow John's suggestions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files: clang/include/clang/Basic/Builtins.def

[PATCH] D79279: Add overloaded versions of builtin mem* functions

gchatelet added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:489 BUILTIN(__builtin_memcpy_inline, "vv*vC*Iz", "nt") +BUILTIN(__builtin_overloaded_memcpy, "v*v*vC*z", "nt") BUILTIN(__builtin_memmove, "v*v*vC*z", "nF") jfb wrote: >

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1605 + })); + }; + I am not a fan of this lambda style, not because I dislike lambdas, but because you've pulled a ton of code that's supporting one or two cases (that could

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 275266. jfb marked 18 inline comments as done. jfb added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:489 BUILTIN(__builtin_memcpy_inline, "vv*vC*Iz", "nt") +BUILTIN(__builtin_overloaded_memcpy, "v*v*vC*z", "nt") BUILTIN(__builtin_memmove, "v*v*vC*z", "nF") gchatelet wrote: >

[PATCH] D79279: Add overloaded versions of builtin mem* functions

erichkeane added a comment. I just took a look at SemaChecking, the rest I'm not sure I get the intent of the patch sufficiently to understand. Comment at: clang/lib/Sema/SemaChecking.cpp:1442 + enum class MemCheckType { Full, Basic }; + Oh boy... all

[PATCH] D79279: Add overloaded versions of builtin mem* functions

gchatelet added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:489 BUILTIN(__builtin_memcpy_inline, "vv*vC*Iz", "nt") +BUILTIN(__builtin_overloaded_memcpy, "v*v*vC*z", "nt") BUILTIN(__builtin_memmove, "v*v*vC*z", "nF") `overloaded` doesn't

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 275005. jfb added a comment. Add memmove and memset (but still missing the codegen tests) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 274948. jfb added a comment. This builtin doesn't return anything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files: clang/include/clang/Basic/Builtins.def Index:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 274949. jfb added a comment. Arcanist ate the rest of my commit and I am confused. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79279/new/ https://reviews.llvm.org/D79279 Files:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

jfb updated this revision to Diff 274847. jfb edited the summary of this revision. jfb added a comment. Overload a new builtin instead. For now I only did memcpy, to get feedback on the approach. I'll add other mem* functions if this makes sense. Repository: rG LLVM Github Monorepo CHANGES