[PATCH] D129075: [AMDGPU] gfx11 allow dlc for MUBUF atomics

Petar Avramovic via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 5 08:34:45 PDT 2022


Petar.Avramovic added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp:216
+        (TSFlags & SIInstrFlags::MUBUF) && AMDGPU::isGFX10(STI))
+      O << " /* dlc is ignored for gfx10 mubuf atomics */";
+    else
----------------
dp wrote:
> I believe the code with `dlc=1` is legal for GFX10, and because `dlc` is ignored, I think we should silently drop it instead of emitting a warning.
> 
> It would be nice to have a test for this case.
I don't have strong opinion here and only wanted to match disassembler error out when `1` is in `dlc bit`.
Assembler/codegen printing `dlc` in asm string but encoding `0` in `dlc bit`, seems misleading to me.

Also `/llvm/include/llvm/IR/IntrinsicsAMDGPU.td` only mentions `// cachepolicy(imm; bit 1 = slc)` but it is possible to force `dlc` or even `glc` on atomic with unused return (selected as no_ret version of the mir atomic).

here is a test:
```
define amdgpu_ps void @noret(<4 x i32> inreg %rsrc, i32 %data, i32 %vindex) {
main_body:
  %out = call i32 @llvm.amdgcn.raw.buffer.atomic.add.i32(i32 %data, <4 x i32> %rsrc, i32 %vindex, i32 0, i32 4)
  ret void
}

declare i32 @llvm.amdgcn.raw.buffer.atomic.add.i32(i32, <4 x i32>, i32, i32, i32)
```
but overall I think that this warning should not be reachable, and that could be avoided by checking value in cache policy operand from IR atomic. I don't know what would be the best place to implement such check.  I think that Matt suggested IR verifier in some discussion.  


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

https://reviews.llvm.org/D129075



More information about the llvm-commits mailing list