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

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

In D104264#2819465 <https://reviews.llvm.org/D104264#2819465>, @koutheir wrote:

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

Not quite. Address spaces in LLVM are a somewhat opaque concept, and can mean anything. I'm just not sure they are exchangeable like you propose on all cases.

For example, the LangRef says of `addrspacecast`: "It can be a no-op cast or a complex value modification, depending on the target and the address space pair."

My assumption was that the back-end should not see address spaces in that way, that they would have been converted by some earlier pass to checks and bounds, but I may be wrong.

How are you generating that IR? Is that something that comes from Clang or another official front-end?

> 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`.

I agree this isn't correct.

I looked at your test and it doesn't generate `LDEX/STREX` for 32-bits, but `LDR/STR` followed by a `DMB`. No address space conversions are needed either way.

The crash only happens with 64-bit types because there's an `i8*` conversion to map `i64` into `{i32, i32}`, so it's accidental. Changing the code to accept any `addrspace` in that cast is fixing the wrong thing.

The only non-GPU test I could find that has "store atomic ... addrspace" is `test/CodeGen/X86/2020_12_02_decrementing_loop.ll` which the end result is an `ud2` (ie. undefined instruction, ie. crash).

I'm not sure the back-end is ready to consume that kind of pattern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104264