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