rsmith added a comment.
These new builtins should ideally support constant evaluation if possible.
================
Comment at: clang/docs/LanguageExtensions.rst:2439-2440
+Clang provides versions of the following functions which are overloaded based
on
+the pointer parameter types:
+
----------------
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.
================
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:
----------------
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).
================
Comment at: clang/include/clang/Basic/Builtins.def:471
// Random GCC builtins
BUILTIN(__builtin_constant_p, "i.", "nctu")
----------------
Are these really GCC builtins?
================
Comment at: clang/include/clang/Basic/Builtins.def:1487
// Clang builtins (not available in GCC).
BUILTIN(__builtin_addressof, "v*v&", "nct")
----------------
The new builtins probably belong in this section of the file instead.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7961-7963
-def err_atomic_op_needs_trivial_copy : Error<
- "address argument to atomic operation must be a pointer to a "
- "trivially-copyable type (%0 invalid)">;
----------------
I'd prefer to keep this diagnostic separate, since it communicates more
information than `err_argument_needs_trivial_copy` does: specifically that we
need a trivial copy because we're performing an atomic operation.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8926-8934
+def err_const_arg : Error<"argument must be non-const, got %0">;
+
+def err_argument_needs_trivial_copy : Error<"address argument must be a
pointer to a trivially-copyable type (%0 invalid)">;
+
+def err_atomic_volatile_unsupported : Error<"mixing _Atomic and volatile
qualifiers is unsupported (%select{%1|%1 and %2}0 cannot have both _Atomic and
volatile)">;
+
+def err_atomic_sizes_must_match : Error<"_Atomic sizes must match, %0 is %1
bytes and %2 is %3 bytes">;
----------------
Please format these diagnostics consistently with the rest of the file: line
break after `Error<`, wrap to 80 columns, don't leave blank lines between
individual diagnostics in a group of related diagnostics.
================
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;
----------------
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.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:5510-5512
+ if ((DstValTy->isAtomicType() || SrcValTy->isAtomicType()) &&
+ (!DstValTy.getUnqualifiedType()->isVoidType() &&
+ !SrcValTy.getUnqualifiedType()->isVoidType()) &&
----------------
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.
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
