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