[PATCH] D104264: Fix atomic loads and stores of 64-bit values in non-default address spaces on ARM

[Koutheir Attouchi via Phabricator via llvm-commits]

koutheir added a comment.

> Basically, what that's saying is that *all* access to *any* address space is the same as an access through the default address space.
>
> It may be true for your case, but not necessarily others. Your code would make it true for all.

Yes, see choice `(1)` above.

> I looked at other targets and none of them assume this is true, except the GPU targets on very specific conditions (I'm, assuming ABI related).

If I understand well, you rather opt for choice `(2)`: Atomic loads and stores are legal only on the default address space.

If that is the case, then I think we should explicitly check for this, and report an error instead of:

- Silently generating code for `i8`, `i16` and `i32`.
- Crashing due to an assertion failure for `i64`.

This choice also makes sense, and I'm ready to implement it. Please confirm that this is your actual opinion.

> This looks to me like the wrong place to fix it. It'd probably be a matter for the middle-end casting the address space to default when performing the atomic load?

That would work around the issue by avoiding the problematic code path. I still think LLVM should provide a consistent behavior irrespective of the integer bit width.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104264