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 <https://reviews.llvm.org/D79279#2240487>, @jfb wrote: > In D79279#2235085 <https://reviews.llvm.org/D79279#2235085>, @rsmith wrote: > >> 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 thing, and a name that contains `atomic` would be more >> obvious. As a concrete alternative, `__atomic_unordered_memcpy` is not much >> longer and seems to have the right kinds of implications. WDYT? > > Kinda, that's the motivating from Hans' paper which I'm following. One other > use case (and the reason I assume Azul folks want it too) is when there's a > GC that looks at objects. With this it knows it won't see torn objects when > the GC is concurrent. It's similar, but generally `atomic` also implies an > ordering, and here it's explicitly unordered (i.e. Bring Your Own Fences). > > So I don't have a strong opinion on the name, but `atomic` seem slightly > wrong. I mean, I see your point, but I think not mentioning `atomic` at all also seems a little surprising, given how tied this feature is to atomic access (eg, rejecting access sizes larger than the inline atomic width). But this is not a hill I have any desire to die on :) If you'd prefer to keep the current name, I can live with it. ================ Comment at: clang/docs/LanguageExtensions.rst:2403-2406 +Constant evaluation support is only provided when the source and destination are +pointers to arrays with the same trivially copyable element type, and the given +size is an exact multiple of the element size that is no greater than the number +of elements accessible through the source and destination operands. ---------------- jfb wrote: > rsmith wrote: > > Is this true in general, or only for `[w]mem{cpy,move}`? I thought for the > > other cases, we required an array of `char` / `wchar_t`? > This is just moving documentation that was below. Phab is confused with the > diff. The documentation that was moved applied to `memcpy`, `memmove`, `wmemcpy`, and `wmemmove`. In the new location, it doesn't apply to `wmemcpy` nor `wmemmove` but does now apply to `memchr`, `memcmp`, ..., for which it is incorrect. We used to distinguish between "string builtins", which had the restriction to character types, and "memory builtins", which had the restriction to trivially-copyable types. Can you put that distinction back, or otherwise restore the wording to the old / correct state? ================ Comment at: clang/docs/LanguageExtensions.rst:2409 +Support for constant expression evaluation for the above builtins can be detected +with ``__has_feature(cxx_constexpr_string_builtins)``. + ---------------- jfb wrote: > rsmith wrote: > > I think this only applies to the above list minus the five functions you > > added to it. Given this and the previous comment, I'm not sure that merging > > the documentation on string builtins and memory builtins is working out > > well -- they seem to have more differences than similarities. > > > > (`memset` is an outlier here that should be called out -- we don't seem to > > provide any constant evaluation support for it whatsoever.) > [w]memset are indeed the odd ones, update says so. This feature test macro doesn't cover `memcpy` / `memmove`; this documentation is incorrect for older versions of Clang. Clang 4-7 define this feature test macro but do not support constant evaluation of `memcpy` / `memmove`. ================ Comment at: clang/lib/CodeGen/CGBuiltin.cpp:636-643 + if (ArgTy->isObjCObjectPointerType()) + return ArgTy->castAs<clang::ObjCObjectPointerType>() + ->getPointeeType() + .isVolatileQualified(); + if (ArgTy->isPointerType()) + return ArgTy->castAs<clang::PointerType>() + ->getPointeeType() ---------------- (Just a simplification, NFC.) ================ Comment at: clang/lib/Sema/SemaChecking.cpp:5609 + return ExprError(Diag(TheCall->getBeginLoc(), + PDiag(diag::err_atomic_op_needs_trivial_copy)) + << DstValTy << DstOp->getSourceRange()); ---------------- jfb wrote: > rsmith wrote: > > Generally, I'm a little uncomfortable about producing an error if a type is > > complete but allowing the construct if the type is incomplete -- that seems > > like a situation where a warning would be more appropriate to me. It's > > surprising and largely unprecedented that providing more information about > > a type would change the program from valid to invalid. > > > > Do we really need the protection of an error here rather than an > > enabled-by-default warning? Moreover, don't we already have a warning for > > `memcpy` of a non-trivially-copyable object somewhere? If not, then I think > > we should add such a thing that also applies to the real `memcpy`, rather > > than only warning on the builtin. > That rationale makes sense, but it's pre-existing behavior for atomic. I can > change all of them in a follow-up if that's OK? > > We don't have such a check for other builtins. I can do a second follow-up to > then adopt these warnings for them too? The pre-existing behavior for atomic builtins is to reject if the type is incomplete. Eg; ``` <stdin>:1:33: error: address argument to atomic operation must be a pointer to a trivially-copyable type ('struct A *' invalid) struct A; void f(struct A *p) { __atomic_store(p, p, 0); } ^ ~ ``` We should do the same here. (Though I'd suggest calling `RequireCompleteType` instead to get a more meaningful diagnostic.) These days I think we should check for unsized types too, eg: ``` if (RequireCompleteSizedType(ScrOp->getBeginLoc(), SrcValTy)) return true; if (!SrcValTy.isTriviallyCopyableType(Context) && !SrcValTy->isVoidType()) return ExprError(...); ``` 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 https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
