rsmith added a comment.
Thanks. I'd like @rjmccall to approve too, but the design of these intrinsics
and the Sema and constant evaluation parts seem fine. (We don't strictly need
constant evaluation to abort on library UB, so I think not catching the
misalignment case is OK.)
================
Comment at: clang/docs/LanguageExtensions.rst:2447-2449
+therefore be a lock-free size for the target architecture. It is a runtime
+constraint violation to provide a memory locations which is aligned to less
than
+the element size. It is a runtime constraint violation to provide a size which
----------------
"runtime constraint violation" is an odd phrase; in C, "constraint violation"
means a diagnostic is required. Can we instead say that it results in undefined
behavior?
================
Comment at: clang/docs/LanguageExtensions.rst:2454
+and might be non-uniform throughout the operation.
+
+The builtins can be used as building blocks for different facilities:
----------------
jfb wrote:
> rsmith wrote:
> > From the above description, I think the documentation is unclear what the
> > types `T` and `U` are used for. I think the answer is something like:
> >
> > """
> > The types `T` and `U` are required to be trivially-copyable types, and
> > `byte_element_size` (if specified) must be a multiple of the size of both
> > types. `dst` and `src` are expected to be suitably aligned for `T` and `U`
> > objects, respectively.
> > """
> >
> > But... we actually get the alignment information by analyzing pointer
> > argument rather than from the types, just like we do for `memcpy` and
> > `memmove`, so maybe the latter part is not right. (What did you intend
> > regarding alignment for the non-atomic case?) The trivial-copyability and
> > divisibility checks don't seem fundamentally important to the goal of the
> > builtin, so I wonder if we could actually just use `void` here and remove
> > the extra checks. (I don't really have strong views one way or the other on
> > this, except that we should either document what `T` and `U` are used for
> > or make the builtins not care about the pointee type beyond its qualifiers.)
> You're right. I've removed most treatment of `T` / `U`, and updated the
> documentation.
>
> I left the trivial copy check, but `void*` is a usual escape hatch.
>
> Divisibility is now only checked for `size` / `element_size`.
Please document the trivial copy check.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2639-2640
case Builtin::BI__builtin_mempcpy: {
+ QualType DestTy = getPtrArgType(CGM, E, 0);
+ QualType SrcTy = getPtrArgType(CGM, E, 1);
Address Dest = EmitPointerWithAlignment(E->getArg(0));
----------------
jfb wrote:
> rsmith wrote:
> > Looking through implicit conversions in `getPtrArgType` here will change
> > the code we generate for cases like:
> >
> > ```
> > void f(volatile void *p, volatile void *q) {
> > memcpy(p, q, 4);
> > }
> > ```
> >
> > ... (in C, where we permit such implicit conversions) to use a volatile
> > memcpy intrinsic. Is that an intentional change?
> I'm confused... what's the difference that this makes for the pre-existing
> builtins? My intent was to get the `QualType` unconditionally, but I can
> conditionalize it if needed... However this ought to make no difference:
> ```
> static QualType getPtrArgType(CodeGenModule &CGM, const CallExpr *E,
> unsigned ArgNo) {
> QualType ArgTy = E->getArg(ArgNo)->IgnoreImpCasts()->getType();
> if (ArgTy->isArrayType())
> return CGM.getContext().getAsArrayType(ArgTy)->getElementType();
> if (ArgTy->isObjCObjectPointerType())
> return ArgTy->castAs<clang::ObjCObjectPointerType>()->getPointeeType();
> return ArgTy->castAs<clang::PointerType>()->getPointeeType();
> }
> ```
> and indeed I can't see the example you provided change in IR from one to the
> other. The issue I'm working around is that getting it unconditionally would
> make ObjC code sad when `id` is passed in as I outlined above.
The example I gave should produce a non-volatile memcpy, and used to do so (we
passed `false` as the fourth parameter to `CreateMemCpy`). With this patch,
`getPtrArgType`will strip off the implicit conversion from `volatile void*` to
`void*` in the argument type, so `isVolatile` below will be `true`, so I think
it will now create a volatile memcpy for the same testcase. If that's not
what's happening, then I'd like to understand why not :)
I'm not saying that's necessarily a bad change, but it is a change, and it's
one we should make intentionally if we make it at all.
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
