llvm / llvm/llvm-project

[mlir][AMDGPU] AMDGPUToROCDL requests `nuw` unconditionally, producing poison for negative-stride memrefs (2 sites)

Open
#221,412 1 comment 0 reactions 0 assignees View on GitHub
mlir
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

## Summary

`AMDGPUToROCDL` asks the shared strided-element helper for
`inbounds | nuw` unconditionally in two lowerings. That flag propagates to
every generated stride multiply and offset add. For a memref with a negative
static stride the multiply genuinely unsigned-wraps, so LLVM is entitled to
treat the address as poison — even though the signed address arithmetic is
correct and the access is in bounds.

Upstream has already established this exact rule and built the guard for it,
but only applied it to two of the four `nuw` requesters:

| caller | guarded? |
|---|---|
| `mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp:52` | yes — requires all strides `>= 0` |
| `mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:280` | yes — asserts `!memref::hasNegativeStaticStride` |
| **`mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp:2359`** | **no** |
| **`mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp:4509`** | **no** |

The rule is stated in `MemRefToLLVM.cpp:44`:

```c++
/// nuw requires every index*stride term to not unsigned-wrap, which holds iff
/// all strides are statically non-negative. Negative strides would make the
/// intermediate mul nuw overflow (e.g., idx * (-1 as u64) wraps for idx > 0).
```

Both sites reproduce at `fdd899123f7b`.

## Running same memref, two lowerings

`memref<128x256xf16, strided<[256, -1], offset: 255>>` is verifier-legal. It
describes reversed rows: logical `[i, j]` maps to physical `255 + i*256 - j`,
so `[0, 255]` is physical element 0 — in bounds, and the signed arithmetic is
correct.

Through the guarded path (`memref.load`, `-finalize-memref-to-llvm`):

```mlir
%7 = llvm.mul %1, %6 overflow : i64
%9 = llvm.mul %0, %8 overflow : i64
%10 = llvm.add %7, %9 overflow : i64
%11 = llvm.getelementptr inbounds %5[%10] ...
```

Through `AMDGPUToROCDL` on the identical layout:

```mlir
%7 = llvm.mul %1, %6 overflow : i64
%9 = llvm.mul %0, %8 overflow : i64 // %8 = -1
%10 = llvm.add %7, %9 overflow : i64
%11 = llvm.getelementptr inbounds|nuw %5[%10] ...
```

`MemRefToLLVM` correctly withholds `nuw`; `AMDGPUToROCDL` asserts it.

## Reproducers

### 1. `amdgpu.global_transpose_load` (`AMDGPUToROCDL.cpp:2359`)

```mlir
func.func @neg_stride(%i : index, %j : index,
%src : memref<128x256xf16, strided<[256, -1], offset: 255>, #gpu.address_space>)
-> vector<8xf16> {
%0 = amdgpu.global_transpose_load %src[%i, %j]
: memref<128x256xf16, strided<[256, -1], offset: 255>, #gpu.address_space> -> vector<8xf16>
return %0 : vector<8xf16>
}
```

```
mlir-opt -convert-amdgpu-to-rocdl=chipset=gfx1250 repro.mlir
```

### 2. `amdgpu.global_prefetch`, non-speculative (`AMDGPUToROCDL.cpp:4509`)

```mlir
func.func @prefetch_neg_stride(
%src : memref<64x64xf16, strided<[64, -1], offset: 63>, #gpu.address_space>,
%i : i64, %j : i64) {
amdgpu.global_prefetch %src[%i, %j] HT WGP
: memref<64x64xf16, strided<[64, -1], offset: 63>, #gpu.address_space>
return
}
```

```
mlir-opt -convert-amdgpu-to-rocdl=chipset=gfx1250 repro.mlir
```

Both emit `llvm.mul ... overflow` against the `-1` stride and a
`getelementptr inbounds|nuw`.

Note the prefetch site already distinguishes speculative mode, which correctly
requests `GEPNoWrapFlags::none`. Only the non-speculative branch is affected.

## Why this is a bug

LLVM LangRef defines `nuw` on integer multiplication as a promise that the
unsigned product fits the result type, and `nuw` on GEP offset arithmetic as a
promise that those operations do not wrap unsigned. Violating either yields
poison.

A negative stride is a large unsigned bit pattern: `255 * (2^64 - 1)` wraps.
The lowering therefore hands LLVM a false promise for an access whose signed
address is correct and in bounds. Nothing in the emitted IR is wrong-looking,
and the code will typically execute correctly today — the defect only manifests
once a consumer exploits the flag.

The `amdgpu.global_transpose_load` ODS requires each source index to be
nonnegative and in bounds for its dimension, matching `memref.load`; index
`[0, 255]` satisfies that for shape `128x256`. So there is no precondition the
input violates.

## Environment

* llvm-project at `fdd899123f7b`
* Release build with assertions, Linux x86-64

Contributor guide

Open the contributing guide

Research direction

Start in mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp at lines 2359 and 4509, and compare the guarded logic in MemRefToLLVM.cpp:44-52. Run each supplied mlir-opt reproducer and inspect the generated LLVM arithmetic flags. Done means negative static strides no longer receive nuw at either affected lowering site, while the existing speculative prefetch behavior remains unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.