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

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8917
+
+def err_atomic_type_must_be_lock_free : Error<"_Atomic type must always be lock-free, %0 isn't">;
+
----------------
rjmccall wrote:
> I don't know why you're adding a bunch of new diagnostics about _Atomic.
Maybe the tests clarify this? Here's my rationale for the 3 new atomic diagnostics:

* Don't support mixing `volatile` and `atomic`, because we'd need to add IR support for it. It might be useful, but as a follow-up.
* Overloaded `memcpy` figures out the atomic operation size based on the element's own size. There's a destination and a source pointer, and we can't figure out the expected atomic operation size if they differ. It's likely an unintentional error to have different sizes when doing an atomic `memcpy`, so instead of figuring out the largest common matching size I figure it's better to diagnose.
* Supporting non-lock-free sizes seems fraught with peril, since it's likely unintentional. It's certainly doable (loop call the runtime support), but it's unclear if we should take the lock just once over the entire loop, or once for load+store, or once for load and once for store. I don't see a point in supporting it.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:636
+  return ArgTy->castAs<clang::PointerType>()->getPointeeType();
+}
+
----------------
rjmccall wrote:
> Since arrays are handled separately now, this is just `getPointeeType()`, but I don't know why you need to support ObjC object pointer types here at all.
I'll remove ObjC handling for now, I added it because of code like what's in:
clang/test/CodeGenObjC/builtin-memfns.m
```
// PR13697
void cpy1(int *a, id b) {
  // CHECK-LABEL: @cpy1(
  // CHECK: call void @llvm.memcpy.p0i8.p0i8.i64(i8* {{.*}}, i8* {{.*}}, i64 8, i1 false)
  memcpy(a, b, 8);
}
```
Should we support this? It seems to me like yes, but you seem to think otherwise?

On arrays / ObjC being handled now: that's not really true... or rather, it now is for the builtins I'm adding, but not for the previously existing builtins. We can't just get the pointer argument type for this code:
```
// <rdar://problem/11314941>
// Make sure we don't over-estimate the alignment of fields of
// packed structs.
struct PS {
  int modes[4];
} __attribute__((packed));
struct PS ps;
void test8(int *arg) {
  // CHECK: @test8
  // CHECK: call void @llvm.memcpy{{.*}} align 4 {{.*}} align 1 {{.*}} 16, i1 false)
  __builtin_memcpy(arg, ps.modes, sizeof(struct PS));
}
```

Because `__builtin_memcpy` doesn't perform the conversion. Arguable a pre-existing bug, which I can patch here as I have, or fix in Sema if you'd rather see that? LMK.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5468
+  bool isAtomic = (DstTy && DstValTy->isAtomicType()) ||
+                  (SrcTy && SrcValTy->isAtomicType());
+
----------------
rjmccall wrote:
> You already know that DstTy and SrcTy are non-null here.
> 
> Why do you need to support atomic types for these operations anyway?  It just seems treacherous and unnecessary.
Leftover from the refactoring :)

It's useful to get atomic memcpy, see https://wg21.link/P1478
It's also part of "support overloaded memcpy" which is what doing more than `volatile` implies.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79279/new/

https://reviews.llvm.org/D79279