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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D97224#2596410 <https://reviews.llvm.org/D97224#2596410>, @rjmccall wrote:

> Do we really consider the existing atomic intrinsics to not impose added alignment restrictions?  I'm somewhat concerned that we're going to produce IR here that's either really suboptimal or, worse, unimplementable, just because we over-interpreted some cue about alignment.  I guess it would only be a significant problem on a target where types are frequently under-aligned for what atomics need, which is not typical, or when the user is doing atomics on a field of something like a packed struct.

For the `__atomic_*` intrinsics, we don't consider those as imposing additional alignment restrictions -- currently, we avoid generating the LLVM IR instruction if it's below natural alignment, since we couldn't specify alignment on the IR instruction. (Now that we have alignment on the LLVM IR operations, I'd like to eventually get rid of that logic from Clang, since it's also handled by LLVM.)

For other intrinsics (e.g. MSVCIntrin::_InterlockedAnd, Builtin::BI__sync_fetch_and_add_4, NVPTX::BI__nvvm_atom_add_gen_i, and the others in those 3 function families), we currently entirely ignore the alignment, and simply assume the argument is naturally-aligned. So, yes, this change could potentially affect the behavior for underaligned types.

So, I could change these to explicitly increase the assumed alignment of their arguments, like I did for InterlockedCompareExchange128. My inclination is not to do so, however...it doesn't seem like a good idea in general to ignore type alignment. But, I'd not be opposed to doing that, if there's a good reason.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97224