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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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:
+
----------------
This is missing some important details:

- What does the size parameter mean? Is it number of bytes or number of elements? If it's number of bytes, what happens if it's not a multiple of the element size, particularly in the `_Atomic` case?
- What does the value parameter to `memset` mean? Is it splatted to the element width? Does it specify a complete element value?
- For `_Atomic`, what memory order is used?
- For `volatile`, what access size / type is used? Do we want to make any promises?
- Are the loads and stores typed or untyped? (In particular, do we annotate with TBAA metadata?)
- Do we guarantee to copy the object representation or only the value representation? (Do we preserve the values of padding bits in the source, and initialize padding bits in the destination?)

You should also document whether constant evaluation of these builtins is supported.


================
Comment at: clang/docs/LanguageExtensions.rst:2446-2448
+These overloads support destinations and sources which are a mix of
+``volatile``, ``_Atomic``, ``restrict``, ``__unaligned``, and use non-default
+address spaces. These can be used as building blocks for different facitilies:
----------------
Mixing those qualifiers doesn't seem like it will work in many cases: we don't allow mixing `volatile` and `_Atomic` (though I'm not sure why; LLVM supports volatile atomic operations), and presumably we shouldn't allow mixing `__unaligned` and `_Atomic` (although I don't see any tests for that, and maybe we should just outright disallow combining `_Atomic` with `__unaligned` in general).


================
Comment at: clang/include/clang/Basic/Builtins.def:471
 
 // Random GCC builtins
 BUILTIN(__builtin_constant_p, "i.", "nctu")
----------------
Are these really GCC builtins?


================
Comment at: clang/include/clang/Basic/Builtins.def:1487
 
 // Clang builtins (not available in GCC).
 BUILTIN(__builtin_addressof, "v*v&", "nct")
----------------
The new builtins probably belong in this section of the file instead.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7961-7963
-def err_atomic_op_needs_trivial_copy : Error<
-  "address argument to atomic operation must be a pointer to a "
-  "trivially-copyable type (%0 invalid)">;
----------------
I'd prefer to keep this diagnostic separate, since it communicates more information than `err_argument_needs_trivial_copy` does: specifically that we need a trivial copy because we're performing an atomic operation.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8926-8934
+def err_const_arg : Error<"argument must be non-const, got %0">;
+
+def err_argument_needs_trivial_copy : Error<"address argument must be a pointer to a trivially-copyable type (%0 invalid)">;
+  
+def err_atomic_volatile_unsupported : Error<"mixing _Atomic and volatile qualifiers is unsupported (%select{%1|%1 and %2}0 cannot have both _Atomic and volatile)">;
+  
+def err_atomic_sizes_must_match : Error<"_Atomic sizes must match, %0 is %1 bytes and %2 is %3 bytes">;
----------------
Please format these diagnostics consistently with the rest of the file: line break after `Error<`, wrap to 80 columns, don't leave blank lines between individual diagnostics in a group of related diagnostics.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1277-1278
                                     CallExpr *Call) {
-  if (Call->getNumArgs() != 1) {
-    S.Diag(Call->getBeginLoc(), diag::err_opencl_builtin_to_addr_arg_num)
-        << Call->getDirectCallee() << Call->getSourceRange();
+  if (checkArgCount(S, Call, 1))
     return true;
 
----------------
There are a bunch of places in this file that do manual argument count checking and could use `checkArgCount` instead (search for `err_typecheck_call_too_` to find them). If you want to clean this up, please do so in a separate change.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5510-5512
+  if ((DstValTy->isAtomicType() || SrcValTy->isAtomicType()) &&
+      (!DstValTy.getUnqualifiedType()->isVoidType() &&
+       !SrcValTy.getUnqualifiedType()->isVoidType()) &&
----------------
Do we need this constraint?

- If one side is atomic and the other is not, then we can do all of the operations with the atomic width.
- If both sides are atomic, then one side is 2^N times the size of the other; we can do 2^N operations on one side for each operation on the other side.

Maybe the second case is not worth the effort, but permitting (for example) a memcpy from an `_Atomic int*` to a `char*` seems useful and there doesn't seem to be a good reason to disallow it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79279/new/

https://reviews.llvm.org/D79279