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

> I'm not an expert on atomics or the various uses of address spaces in the wild, but I'm curious as to what your use case is.

The use of a different address space in my case is used to distinguish "pointers to native memory" from "pointers to managed objects" in a Java virtual machine.

> This doesn't sound like the right solution.
>
> Maybe I'm misunderstanding what you're trying to do, but if the address space is different because it's in a different virtual environment (encapsulation, security, hardware purposes), then casting it to the default address space won't point to the "right" element.

I agree with your concern, and that's the reason why I explicitly stated my assumption:

> This patch assumes that ARM synchronization semantics are the same irrespective of the address space.

In my use case, the address space cast works as expected.

I thought my assumption was reasonable because that is the same assumption made when performing atomic loads and stores on other integers with lower bit widths, e.g., `i32`, `i16`, `i8`. For instance, replacing all `i64` with `i32` in the minimal test below works perfectly without this patch. This patch simply makes the same operation work for `i64` values.

I think that atomic loads and stores from non-default address spaces should behave uniformly irrespective of the integer bit width (as long as it is supported by the processor). This means choosing:

1. Either they behave the same way irrespective of the address space, or
2. They are legal only on the default address space.

The current situation is somewhere in between, and this patch brings it completely to choice `(1)`.



================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:19696
+
+    unsigned AddressSpace =
+        cast<PointerType>(Addr->getType())->getAddressSpace();
----------------
rengolin wrote:
> Copy and paste introduce errors. Please, factor this in a new anonymous namespace helper.
OK. I'll do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104264