[PATCH] D97224: Use Address for CGBuilder's CreateAtomicRMW and CreateAtomicCmpXchg.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

> The idea here is not to "ignore type alignment".  `EmitPointerWithAlignment` will sometimes return an alignment for a pointer that's less than the alignment of the pointee type, e.g. because you're taking the address of a packed struct field.  The critical question is whether the atomic builtins ought to make an effort to honor that reduced alignment, even if it leads to terrible code, or if we should treat the use of the builtin as a user promise that the pointer is actually more aligned than you might think from the information statically available.

Agreed -- that is the question. In general, I'd like to default to basing decisions only on the statically-known alignments, because I think that'll typically be the best choice for users. Where there's something like a packed struct, it's likely that the values will end up under-aligned in fact, not just in the compiler's understanding.

> For example, the MSDN documentation for `InterlockedIncrement` says that it requires 32-bit alignment [...].  I would say that all of the Interlocked APIs ought to be read as guaranteeing the natural, full-width alignment for their operation.

I had missed that it was documented in some of the other functions (beyond InterlockedCompareExchange128). I'll change the remainder of the _Interlocked APIs to assume at least natural alignment.

> I'm less certain about what to do with the `__atomic_*` builtins

The `__atomic` builtins have already been supporting under-aligned pointers all along -- and that behavior is unchanged by this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97224