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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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