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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
