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 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?
================
Comment at: clang/docs/LanguageExtensions.rst:2395-2397
+Beyond C's specification for these functions, the above builtins can also be
+overloaded on non-default address spaces. Further, the following builtins can
+also be overloaded on ``volatile``:
----------------
"can also be overloaded" -> "are also overloaded" would be clearer I think.
("Can also be overloaded" would suggest to me that it's the user of the builtin
who overloads them, perhaps by declaring overloads.)
================
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.
----------------
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`?
================
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)``.
+
----------------
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.)
================
Comment at: clang/docs/LanguageExtensions.rst:2451
+lock-free size for the target architecture. It is undefined behavior to provide
+a memory locations which is aligned to less than the access size. It is
+undefined behavior to provide a size which is not evenly divided by the
----------------
================
Comment at: clang/docs/LanguageExtensions.rst:2452
+a memory locations which is aligned to less than the access size. It is
+undefined behavior to provide a size which is not evenly divided by the
+specified access size.
----------------
================
Comment at: clang/lib/AST/ExprConstant.cpp:8870
+ return false;
+ if (N.urem(ElSz.getLimitedValue()) != 0) {
+ Info.FFDiag(E, diag::note_constexpr_mem_sized_bad_size)
----------------
`getLimitedValue()` here seems unnecessary; `urem` can take an `APInt`.
================
Comment at: clang/lib/AST/ExprConstant.cpp:8872
+ Info.FFDiag(E, diag::note_constexpr_mem_sized_bad_size)
+ << (int)N.getLimitedValue() << (int)ElSz.getLimitedValue();
+ return false;
----------------
Consider using `toString` instead of truncating each `APSInt` to `uint64_t`
then to `int`. The size might reliably fit into `uint64_t`, but I don't think
we can assume that `int` is large enough.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:635
+ return ArgTy->castAs<clang::ObjCObjectPointerType>()->getPointeeType();
+ return ArgTy->castAs<clang::PointerType>()->getPointeeType();
+}
----------------
I'm not sure this `castAs` is necessarily correct. If the operand is C++11
`nullptr`, we could perform a null-to-pointer implicit conversion, and `ArgTy`
could be `NullPtrTy` after stripping that back off here.
It seems like maybe what we want to do is strip off implicit conversions until
we hit a non-pointer type, and take the pointee type we found immediately
before that?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:5609
+ return ExprError(Diag(TheCall->getBeginLoc(),
+ PDiag(diag::err_atomic_op_needs_trivial_copy))
+ << DstValTy << DstOp->getSourceRange());
----------------
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.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:5732
+
+ if (!DstValTy.isTriviallyCopyableType(Context) && !DstValTy->isVoidType())
+ return ExprError(Diag(TheCall->getBeginLoc(),
----------------
jfb wrote:
> rsmith wrote:
> > You need to call `Sema::isCompleteType` first before asking this question,
> > in order to trigger class instantiation when necessary in C++. (Likewise
> > for the checks in the previous function.)
> Before the condition, right? LMK if I added the right thing!
It would be more correct from a modules perspective to use
```
if (isCompleteType(Loc, T) && !T.isTriviallyCopyableType(Context))
```
That way, if the definition of the type is in some loaded-but-not-imported
module file, we'll treat it the same as if the definition of the type is
entirely unknown. (That also removes the need to check for the `void` case.)
But given that this only allows us to accept code that is wrong in some sense,
I'm not sure it really matters much.
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
