This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][AMDGPU] Add AMDGPU dialect, wrappers around raw buffer intrinsics
ClosedPublic

Authored by krzysz00 on Mar 30 2022, 2:57 PM.

Details

Summary

By analogy with the NVGPU dialect, introduce an AMDGPU dialect for
AMD-specific intrinsic wrappers.

The dialect initially includes wrappers around the raw buffer intrinsics.

On AMD GPUs, a memref can be converted to a "buffer descriptor" that
allows more precise control of memory access, such as by allowing for
out of bounds loads/stores to be replaced by 0/ignored without adding
additional conditional logic, which is important for performance.

The repository currently contains a limited conversion from
transfer_read/transfer_write to Mubuf intrinsics, which are an older,
deprecated intrinsic for the same functionality.

The new amdgpu.raw_buffer_* ops allow these operations to be used
explicitly and for including metadata such as whether the target
chipset is an RDNA chip or not (which impacts the interpretation of
some bits in the buffer descriptor), while still maintaining an
MLIR-like interface.

(This change also exposes the floating-point atomic add intrinsic.)

Diff Detail

Event Timeline

krzysz00 created this revision.Mar 30 2022, 2:57 PM
Herald added a project: Restricted Project. · View Herald Transcript
krzysz00 requested review of this revision.Mar 30 2022, 2:57 PM
bondhugula added inline comments.Apr 4 2022, 7:18 PM
mlir/test/Target/LLVMIR/rocdl.mlir
195–209

Interspersing the CHECK lines here makes it harder to read the test case. Could you group the CHECK lines and the input test case lines separately here (and in rocdl.mlir above)?

krzysz00 updated this revision to Diff 420565.Apr 5 2022, 9:54 AM
  • Reorganize CHECK lines in LLVM tests
krzysz00 marked an inline comment as done.Apr 5 2022, 9:56 AM

One question: do y'all think targetIsRDNA makes more sense as a pass option or as an attribute?

mlir/test/Target/LLVMIR/rocdl.mlir
195–209

Done - and I also cleaned up a test for the older buffer intrinsic that had the same issue.

krzysz00 marked an inline comment as done.Apr 29 2022, 9:51 AM

@bondhugula and others - after the NVGPU dialect decision, would y'all rather these ops go into an analogous AMDGPU dialect?

@bondhugula and others - after the NVGPU dialect decision, would y'all rather these ops go into an analogous AMDGPU dialect?

That would be my preference as those ops are very target specific.

krzysz00 updated this revision to Diff 426832.May 3 2022, 2:18 PM
krzysz00 retitled this revision from [MLIR][GPU][AMD] Introduce wrappers around raw buffer intrinsics to [MLIR][AMDGPU] Add AMDGPU dialect, wrappers around raw buffer intrinsics.
krzysz00 edited the summary of this revision. (Show Details)

Move ops into a new AMDGPU dialect

ThomasRaoux accepted this revision.May 6 2022, 1:56 PM

This looks good to me once the minor comments I added are addressed. I'm not familiar with the semantic of the ops added so I didn't review it in details but it overall makes sense to me.

mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp
2

Update the comment

30

nit: you can just do rewriter.getI32Type() this way you could also make this a static helper outside the struct

225

nit: getResult(0)

utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
3684–3686

remove extra lines

3707

what happened to NVGPUToNVVM library? I assume you didn't mean to remove it?

This revision is now accepted and ready to land.May 6 2022, 1:56 PM

Hi! I found one misprint :)

mlir/include/mlir/Dialect/AMDGPU/AMDGPU.td
143
krzysz00 marked 4 inline comments as done.May 9 2022, 8:33 AM
krzysz00 added inline comments.
mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp
2

... thought I got them all, whoops!

utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
3707

Whoops.

krzysz00 updated this revision to Diff 428097.May 9 2022, 8:34 AM
krzysz00 marked 2 inline comments as done.

Address review comments

krzysz00 updated this revision to Diff 428168.May 9 2022, 12:35 PM

Fix Bazel build

krzysz00 updated this revision to Diff 428191.May 9 2022, 1:36 PM

Maybe this'll fix the bazel build?