This is an archive of the discontinued LLVM Phabricator instance.

[mlir][gpu] Add passes to attach (NVVM|ROCDL) target attributes to GPU Modules
ClosedPublic

Authored by fmorac on Aug 7 2023, 7:25 PM.

Details

Summary

Adds the passes nvvm-attach-target & rocdl-attach-target for attaching nvvm.target` & rocdl.target attributes to GPU Modules.

These passes search GPU Modules in the immediate region of the Op being acted on, attaching the target attribute to the module.
Modules can be selected using a regex string, allowing fine grain attachment of targets, see the test attach-target.mlir for an example.

Depends on D154153

Diff Detail

Event Timeline

fmorac created this revision.Aug 7 2023, 7:25 PM
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
fmorac published this revision for review.Aug 7 2023, 7:30 PM
fmorac edited the summary of this revision. (Show Details)
fmorac added reviewers: mehdi_amini, krzysz00.
mehdi_amini added inline comments.Aug 8 2023, 10:23 PM
mlir/lib/Dialect/LLVMIR/Transforms/NVVMAttachTarget.cpp
70 ↗(On Diff #548034)

Nit, inverse the condition, use continue;, and reduce indentation

mlir/lib/Dialect/LLVMIR/Transforms/ROCDLAttachTarget.cpp
61 ↗(On Diff #548034)

Don't we have a C++ FastMathFlag class with enums, the string interface is... not great in general.

fmorac added inline comments.Aug 9 2023, 11:07 AM
mlir/lib/Dialect/LLVMIR/Transforms/ROCDLAttachTarget.cpp
61 ↗(On Diff #548034)

Yes, but these have a more direct meaning in AMDGCN, these are names of control variables

https://github.com/RadeonOpenCompute/ROCm-Device-Libs/blob/amd-stg-open/doc/OCML.md#controls

I'd need to change the syntax in the attribute, but I can change it to use the arith Fast Math enum.

mehdi_amini added inline comments.Aug 9 2023, 9:50 PM
mlir/lib/Dialect/LLVMIR/Transforms/ROCDLAttachTarget.cpp
61 ↗(On Diff #548034)

Forget it, I misread the context here.

fmorac updated this revision to Diff 549340.Aug 11 2023, 4:24 AM

Addressed the comments.

The extra changes to LLVM Transforms are because the shared lib build failed in
the previous version -there was a cyclic dependency.

The issue was caused by LLVMTranslation using LLVMTransforms and NVVMTarget using libs in LLVMTranslation.
The solution is to move out the function needed by LLVMTranslation to a separete utils lib.

mehdi_amini added inline comments.Aug 11 2023, 11:11 AM
mlir/lib/Dialect/LLVMIR/Transforms/CMakeLists.txt
24 ↗(On Diff #549340)

It seems a bit surprising to me that LLVM transforms would depend on the GPU stuff.
Seems like this new pass should move elsewhere instead?

fmorac added inline comments.Aug 11 2023, 11:15 AM
mlir/lib/Dialect/LLVMIR/Transforms/CMakeLists.txt
24 ↗(On Diff #549340)

I added it here because it was related to NVVM & ROCDL, but I'll move it to GPU.

fmorac updated this revision to Diff 549471.EditedAug 11 2023, 11:36 AM
fmorac marked an inline comment as done.
fmorac retitled this revision from [mlir][NVVM|ROCDL] Add passes to attach target attributes to GPU Modules to [mlir][gpu] Add passes to attach (NVVM|ROCDL) target attributes to GPU Modules.

Moved passes to GPU.

mehdi_amini accepted this revision.Aug 11 2023, 12:51 PM
This revision is now accepted and ready to land.Aug 11 2023, 12:51 PM