llvm / llvm/llvm-project

[AMDGPU] AtomicOptimizer reassociates uniform atomicrmw fadd without any opt-in

Open
#223,608 9 comments 0 reactions 0 assignees View on GitHub
backend:AMDGPU
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

For a uniform pointer and value, `AMDGPUAtomicOptimizer` combines the `fadd` contributions of the active lanes in a wavefront into one atomic by multiplying the value by the active-lane count. This can produce a final value in memory that no legal execution of the original atomics can produce. Is this intended to be a legal transform without any opt-in, and if so, where is that documented ? I would like to know before extending the pass.

Minimal input, uniform pointer and value:

```llvm
target triple = "amdgcn-amd-amdhsa"

define amdgpu_kernel void @add_ones(ptr addrspace(1) %counter) {
%old = atomicrmw fadd ptr addrspace(1) %counter, float 1.000000e+00 monotonic
ret void
}
```

```sh
opt -S -mtriple=amdgcn-amd-amdhsa -mcpu=gfx1201 \
-passes='amdgpu-atomic-optimizer' add-ones.ll
```

produces (declarations and attributes trimmed):

```llvm
define amdgpu_kernel void @add_ones(ptr addrspace(1) %counter) #0 {
entry:
%0 = call i32 @llvm.amdgcn.ballot.i32(i1 true)
%1 = call i32 @llvm.amdgcn.mbcnt.lo(i32 %0, i32 0)
%2 = call i32 @llvm.ctpop.i32(i32 %0)
%3 = uitofp i32 %2 to float
%4 = fmul float 1.000000e+00, %3
%5 = icmp eq i32 %1, 0
br i1 %5, label %6, label %8

6:
%7 = atomicrmw fadd ptr addrspace(1) %counter, float %4 monotonic, align 4
br label %8

8:
ret void
}
```

Initialize the counter to `16777216.0f` (`2^24`), launch one block of two work-items, and read the counter after the kernel completes. Every legal execution performs two separate RMWs, each of which rounds `2^24 + 1.0f` back to `2^24` under round-to-nearest-even, the default LLVM FP environment, so the only possible final value is `16777216` (`0x4b800000`). The transformed program adds `2.0f` once and leaves `16777218` (`0x4b800001`). The multiplication is exact here, and repeated addition of the contributions would also give exactly `2.0f`; the issue is reassociating the contributions with the value already in memory:

```text
Original: (memory + contribution) + contribution
Transformed: memory + (contribution + contribution)
```

This is not specific to the `2^24` integer-precision threshold. Starting from `1000.0f`, adding `0.03f` twice gives `1000.06005859375` serially and `1000.05999755859375` combined. The combined value is closer to the real-number sum, but LangRef's Floating-Point Semantics section says optimizations may not change the observed bitwise result of these operations, and `atomicrmw` cannot carry the flags that would relax that.

Nothing in the input opts in to this. `atomicrmw` cannot carry fast-math flags, the function has no `unsafe-fp-math` attribute, and there is no `!amdgpu.*` atomic metadata (which in any case only covers memory kind and denormal mode). The pass does not check any of these; its only FP-environment query is `strictfp`, which changes whether the `fmul` is emitted as a constrained operation, not whether the rewrite happens. `fsub` takes the same path; `fmin`/`fmax` are idempotent and unaffected. The divergent-value path reassociates too (tree reduction in `buildReduction`/`buildScan`), but there the original program admits many serial results; the uniform path is the case where it admits exactly one.

Observed on gfx1201 with a Release+assertions build of `dbac421efa19` (unchanged at `10b2437880e6`), wave32 and wave64, compiling the `opt` output with `llc -O3 -mcpu=gfx1201 -amdgpu-atomic-optimizer-strategy=None` (so the pass runs exactly once) and launching one block of N work-items with `hipModuleLaunchKernel`:

| Work-items | Strategy None | DPP | Iterative |
| ---: | ---: | ---: | ---: |
| 2 | 16777216 | 16777218 | 16777218 |
| 256 | 16777216 | 16777472 | 16777472 |

The value is uniform, so both strategies take the existing uniform-value path. In the 256-work-item case the aggregation happens per wavefront, not once across the block. The reproducer does not use the returned value.

This is the default pipeline, not an opt-in: HIP `atomicAdd(float*, 1.0f)` compiled with ROCm clang at `-O2` (no `-munsafe-fp-atomics`) produces the same `s_bcnt1_i32_b32` / `v_cvt_f32_ubyte0` / `global_atomic_add_f32` sequence.

This was raised when D156301 was reviewed. [jayfoad noted](https://reviews.llvm.org/D156301#4579454) that an `fmul` does not in general match a sequence of `fadd`s. [arsenm noted](https://reviews.llvm.org/D156301#4612013) that atomics carry no fast-math flags and that an opt-in would be needed to do anything other than the add sequence. The patch was then [accepted](https://reviews.llvm.org/D156301#4626650) with:

> I suppose this is fine. You didn't have any adding order guarantee before

The example above is meant to address that directly. An unspecified order permits any result that some serial ordering of the original RMWs can produce; it does not by itself permit changing their parenthesization. With a uniform value every ordering produces the same result, so in the two-work-item case that set is exactly `{16777216}`, and `16777218` is not in it.

Later, #96479 disabled the uniform-value optimization when the returned value was used, and #97604 re-enabled it with fixes to the returned value for the Inf, NaN and `-0.0` cases. Those changes addressed the value returned by the optimized atomic, not the value written to memory.

If this is intended, I can send a patch documenting it in AMDGPUUsage. If an opt-in is needed, should it be per-instruction metadata alongside the existing `!amdgpu.no.fine.grained.memory` / `!amdgpu.ignore.denormal.mode` family, a function attribute, or something else ? I am extending this pass and want to follow the same rule.

CC @arsenm @jayfoad @yxsamliu @ssahasra

Contributor guide

Open the contributing guide

Research direction

Start with AMDGPUAtomicOptimizer, especially the uniform-value path and buildReduction/buildScan, then review the LangRef Floating-Point Semantics and the D156301 discussion. Reproduce the issue with the supplied opt command and LLVM IR. Done means establishing whether the transform needs documentation, an opt-in, or a correctness fix, with the chosen policy reflected in relevant tests or AMDGPUUsage.

Written by the indexing model from the issue text.

Assessment

Domain
compilers
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.