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

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

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 = <unspecified>)``
+* ``__builtin_memmove_overloaded(QUAL void *dst, QUAL const void *src, size_t byte_size, size_t byte_element_size = <unspecified>)``
+* ``__builtin_memset_overloaded(QUAL void *dst, unsigned char val, size_t byte_size, size_t byte_element_size = <unspecified>)``
----------------
What happens if `byte_element_size` does not divide `byte_size`?


================
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 = <unspecified>)``
+* ``__builtin_memmove_overloaded(QUAL void *dst, QUAL const void *src, size_t byte_size, size_t byte_element_size = <unspecified>)``
+* ``__builtin_memset_overloaded(QUAL void *dst, unsigned char val, size_t byte_size, size_t byte_element_size = <unspecified>)``
----------------
rsmith wrote:
> What happens if `byte_element_size` does not divide `byte_size`?
Did you really mean `void*` here? I've been pretty confused by some of the stuff happening below that seems to depend on the actual type of the passed pointer, which would make more sense if you meant `QUAL T *` here rather than `QUAL void*`. Do the builtins do different things for different argument pointee types or not?


================
Comment at: clang/docs/LanguageExtensions.rst:2444-2445
+parameter can be provided to specify element access size in bytes. When the
+element size is provided, the memory will be accessed with a sequence of
+operations of size equal to or a multiple of the pointer's element size. The
+order of operations is unspecified, and each access has unordered atomic
----------------
"the pointer's element size" -- do you mean "the provided element size"?

Does the element size need to be a compile-time constant? (Presumably, but you don't say so.)


================
Comment at: clang/docs/LanguageExtensions.rst:2446-2447
+operations of size equal to or a multiple of the pointer's element size. The
+order of operations is unspecified, and each access has unordered atomic
+semantics. This means that reads and writes do not tear at the individual
+element level, and they each occur exactly once, but the order in which they
----------------
Presumably this means that it's an error if we don't provide lock-free atomic access for that size. Would be worth saying so.


================
Comment at: clang/docs/LanguageExtensions.rst:2450-2451
+occur (and in which they are observable) can only be guaranteed using
+appropriate fences around the function call. Atomic memory operations must be to
+memory locations which are aligned to no less than the element size.
+
----------------
What happens if they're not? Is it UB, or is it just not guaranteed to be atomic?


================
Comment at: clang/docs/LanguageExtensions.rst:2456
+
+These can be used as building blocks for different facitilies:
+
----------------
*facilities


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8937-8938
+  "(%0 is volatile)">;
+def err_elsz_on_sizeless : Error<
+  "element size must not be on a sizeless type (%0 invalid)">;
+def err_elsz_must_be_lock_free : Error<
----------------
Given the new documentation, I would expect you don't need this any more.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8941-8943
+def err_atomic_builtin_ext_size_mismatches_el : Error<
+  "number of bytes to copy must be a multiple of pointer element size, "
+  "got %0 bytes to copy with element size %1 for %2">;
----------------
Presumably the number of bytes need not be a compile-time constant? It's a bit weird to produce an error rather than a warning on a case that would be valid but (perhaps?) UB if the argument were non-constant.


================
Comment at: clang/lib/AST/ExprConstant.cpp:8793
+                BuiltinOp == Builtin::BI__builtin_memmove_overloaded ||
                 BuiltinOp == Builtin::BI__builtin_wmemmove;
 
----------------
jfb wrote:
> If we end up making alignment a runtime constraint, then I'll need to check it in consteval. Otherwise I don't think we need to check anything since Sema ought to have done all the required checks already.
I don't see how you can check the alignment at compile time given a `void*` argument.

We presumably need to check that the element size (if given) divides the total size, assuming the outcome is UB if not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279