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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D79279#2197118 <https://reviews.llvm.org/D79279#2197118>, @rsmith wrote:

> Two observations that are new to me in this review:
>
> 1. We already treat all builtins as being overloaded on address space.
> 2. The revised patch treats `__builtin_mem*_overloaded` as being overloaded *only* on address space, volatility, and atomicity. (We've tuned the design to a point where the other qualifiers don't matter any more.)
>
> So, we're adding three features here: overloading on (a) address space, (b) volatility, and (c) atomicity. (a) is already available in the non-`_overloaded` form, and we seem to be approaching agreement that (b) should be available in the non-`_overloaded` form too. So that only leaves (c), which is really not `_overloaded` but `_atomic`.
>
> Based on those observations I'm now thinking that we might prefer a somewhat different approach (but one that should require only minimal changes to the patch in front of us). Specifically:
>
> 1. Stop treating lib builtins (eg, plain `memcpy`) as overloaded on address space. That's a (pre-existing) conformance bug, at least for the Embedded C TR.
> 2. Keep `__builtin_` forms of lib builtins overloaded on address space. (No change.)
> 3. Also overload `__builtin_` forms of lib builtins on volatility where it makes sense, instead of adding new builtin names `__builtin_mem*_overloaded`.
> 4. Add a new name for the builtin for the atomic forms of `memcpy` and `memset` (`__builtin_memcpy_unordered_atomic` maybe?).
> 5. Convert the "trivial types" check from an error to a warning and apply it to all the mem* overloads. (Though I think we might actually already have such a check, so this might only require extending it to cover the atomic builtin.)
>
> What do you think?

That's fine with me, but as John noted that's inconsistent with what he thought builtins allowed (i.e. `#define memcpy(dst, src, sz) __builtin_memcpy(dst, src, sz)`. If that's Not A Thing then your plan works. If it is then we need to tune it a bit.

Also note that the `_atomic` builtin also needs to support some overloading, at least for address spaces (and maybe `volatile` in the future).

So, let me know what you'd both rather see, so I don't ping-pong code too much.



================
Comment at: clang/docs/LanguageExtensions.rst:2434
+* ``volatile``
+* ``restrict``
+* ``__unaligned``
----------------
rsmith wrote:
> Does `restrict` really make sense here? It seems like the only difference it could possibly make would be to treat `memcpy` as `memmove` if either operand is marked restrict, but
> (a) if the caller wants that, they can just use `memcpy` directly, and
> (b) it's not correct to propagate restrict-ness from the caller to the callee anyway, because restrict-ness is really a property of the declaration of the identifier in its scope, not a property of its type:
> ```
> void f(void *restrict p) {
>   __builtin_memmove(p, p + 1, 4);
> }
> ```
> (c) a restrict qualifier on the pointee type is irrelevant to memcpy and a restrict qualifier on the pointer type isn't part of QUAL.
I dropped `restrict`.


================
Comment at: clang/docs/LanguageExtensions.rst:2435
+* ``restrict``
+* ``__unaligned``
+* non-default address spaces
----------------
rsmith wrote:
> I don't think ``__unaligned`` matters any more. We always take the actual alignment inferred from the pointer arguments, just like we do for non-overloaded `memcpy`.
It's still allowed as a qualifier, though.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5732
+
+    if (!DstValTy.isTriviallyCopyableType(Context) && !DstValTy->isVoidType())
+      return ExprError(Diag(TheCall->getBeginLoc(),
----------------
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279