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

[JF Bastien via Phabricator via cfe-commits]

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 have alignments of at least the element size. That makes sense when the IR is only ever built internally to LLVM, but now that I'm adding a builtin it's more of a dynamic property. Should I also force this in the frontend (understanding that alignment isn't always well known at compile time), or should simply assume that the alignment is correct because it's a dynamic property?

I left some FIXMEs in the CodeGen test for this.



================
Comment at: clang/docs/LanguageExtensions.rst:2439-2440
 
+Clang provides versions of the following functions which are overloaded based on
+the pointer parameter types:
+
----------------
rsmith wrote:
> 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.
Most of these are answered in the update.

Some of the issue is that the current documentation is silent on these points already, by saying "same as C's `mem*` function". I'm relying on that approach here as well.

Size is bytes.

`memset` value is an `unsigned char`.

Memory order is unordered, and accesses themselves are done in indeterminate order.

For `volatile`, it falls out of the new wording that we don't provide access size guarantees. We'd need to nail down IR better to do so, and I don't think it's the salient property (though as discussed above, it might be useful, and the `element_size` parameter make it easy to do so).

Same on TBAA, no mention because "same as C" (no TBAA annotations).

Same on copying bits as-is.

Good point on constant evaluation. I added support. Note that we don't have `memset` constant evaluation, so I didn't support it. Seems easy, but ought to be a separate patch.


================
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:
----------------
rsmith wrote:
> 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).
`volatile` and `_Atomic` ought to work...

For this code I didn't make it work (even if it might be useful), because we'd need IR support for it.

On mixing `_Atomic __unaligned`: I left a FIXME because I'm not 100% sure, given the alignment discussion on atomic in general. Let's see where we settle: if we make it a pure runtime property then `__unaligned` ought to be fine because it's a constraint violation if the actual pointer is truly unaligned.


================
Comment at: clang/include/clang/Basic/Builtins.def:471
 
 // Random GCC builtins
 BUILTIN(__builtin_constant_p, "i.", "nctu")
----------------
rsmith wrote:
> Are these really GCC builtins?
Oops, I didn't see that comment, was just copying `__builtin_memcpy_inline`. I'll move it too.


================
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;
 
----------------
rsmith wrote:
> 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.
D84666


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5510-5512
+  if ((DstValTy->isAtomicType() || SrcValTy->isAtomicType()) &&
+      (!DstValTy.getUnqualifiedType()->isVoidType() &&
+       !SrcValTy.getUnqualifiedType()->isVoidType()) &&
----------------
rsmith wrote:
> 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.
Based on @rjmccall's feedback, I'm disallowing `_Atomic` qualification, and keying off the optional `element_size` parameter to determine atomicity. I'm also only taking in one size, not two, since as discussed it might be useful to allow two but I haven't heard that anyone actually wants it at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279