[llvm-branch-commits] [llvm] AMDGPU: Start considering new atomicrmw metadata on integer operations (PR #122138)

[Yaxun Liu via llvm-branch-commits]

yxsamliu wrote:

> > Clang adds !amdgpu.no.fine.grained.memory and !amdgpu.no.remote.memory to any atomic instructions by default. I think this behavior is expected to keep ISA unchanged compared to the ISA before these metatadat were introduced. Did I miss anything?
> 
> All of the tests that fail are atomicMin_system and atomicMax_system. I would expect that the explicit system scoped functions would not be using these annotations. With this PR, these tests are switching from CAS expansion to the direct instruction.
> 
> It just happens that the integer min and max are the cases we handled conservatively before, so it's possible the test is just wrong in some way

I investigated a similar issue about `__hip_atomic_fetch_max` for float on gfx1100.

https://github.com/ROCm/hip-tests/blob/amd-staging/catch/unit/atomics/__hip_atomic_fetch_max.cc#L46

Basically it sets the original memory with 5.5 and do an atomic float max with 7.5, so the expected value in the memory should be 7.5 but we got 5.5.

It was triggered by my clang change to add no_remote_memory and no_fine_grained_memory to atomicRMW max instruction. Before my change, the backend emits global_atomic_cmpswp_b32. After my change, the backend emits global_atomic_max_f32.

The test tried shared memory and global memory allocated in different ways: 

https://github.com/ROCm/hip-tests/blob/amd-staging/catch/include/resource_guards.hh#L63

shared memory passes

memory allocated by hipMalloc, malloc/hipHostRegister, hipMallocManaged passes

only memory allocated by hipHostMalloc fails

I think the difference is that hipHostMalloc allocates fine-grained memory, so it violates the requirement no_fine_grained_memory imposed on the atomic max instruction.

If I add [[clang::atomic(fine_grained_memory)]] to the block that calls `__hip_atomic_fetch_max` (https://github.com/ROCm/hip-tests/blob/amd-staging/catch/unit/atomics/min_max_common.hh#L85), the test passes. I think this verifies that the atomic attribute works.

https://github.com/llvm/llvm-project/pull/122138