This is an archive of the discontinued LLVM Phabricator instance.

[mlir][gpu] Refactor ConvertGpuLaunchFuncToCudaCalls pass.
ClosedPublic

Authored by whchung on May 18 2020, 3:46 PM.

Details

Summary

Due to similar APIs between CUDA and ROCm (HIP),
ConvertGpuLaunchFuncToCudaCalls pass could be used on both platforms with some
refactoring.

In this commit:

  • Migrate ConvertLaunchFuncToCudaCalls from GPUToCUDA to GPUCommon, and rename.
  • Change CUDA-specific runtime wrapper APIs be specifiable as PassOptions.
  • Naming changes within the implementation and tests.

Subsequent patches would introduce ROCm-specific tests and runtime wrapper
APIs.

Diff Detail

Event Timeline

whchung created this revision.May 18 2020, 3:46 PM
Herald added a project: Restricted Project. · View Herald Transcript

As an alternative, we could stick with fixed names, replace the mcu prefix by something neutral and then for AMD use case we link a different helper library in the tests. WDYT?

mlir/lib/Conversion/GPUCommon/ConvertLaunchFuncToRuntimeCalls.cpp
103–105

There is a CUDA left-over here.

whchung updated this revision to Diff 264925.May 19 2020, 8:12 AM

Revise the patch addressing part of the code review comments.

As an alternative, we could stick with fixed names, replace the mcu prefix by something neutral and then for AMD use case we link a different helper library in the tests. WDYT?

@herhut I revised the patch so:

  • runtime wrapper APIs have more platform-neutral names.
  • GPU binary blobs still carry platform-specific attributes, and could be specified as a PassOption. If none specified, default would still be nvvm.cubin.
  • Also it seems nvvm.cubingetter is no longer used so I removed it from this patch.
whchung updated this revision to Diff 264972.May 19 2020, 10:59 AM
whchung marked an inline comment as done.

Amend unit test for ROCm-specific checks.

Thank you!

mlir/include/mlir/Conversion/GPUCommon/GPUCommonPass.h
2

Please fix comment.

mlir/lib/Conversion/GPUCommon/CMakeLists.txt
20

Please remove commented-out lines.

mlir/lib/Conversion/GPUCommon/ConvertLaunchFuncToRuntimeCalls.cpp
187–188

Maybe remove the CUDA reference here, too.

mlir/test/Conversion/GPUCommon/lower-launch-func-to-gpu-runtime-calls.mlir
1–2

Move this file to GPUCommon directory.

whchung updated this revision to Diff 265235.May 20 2020, 7:25 AM
whchung marked 4 inline comments as done.

Review code comments.

whchung marked an inline comment as done.May 20 2020, 7:28 AM

@herhut Revised the patch per your review comments. I left gpuBinaryAnnotation as a PassOption because in my use case I would invoke LLVM multiple times to trigger code generation for different targets / features, and I use this attribute to help me tell which binary blob comes from which path.

mlir/test/Conversion/GPUCommon/lower-launch-func-to-gpu-runtime-calls.mlir
1–2

this file is in GPUCommon now.

herhut accepted this revision.May 20 2020, 9:35 AM

Thanks!

This revision is now accepted and ready to land.May 20 2020, 9:35 AM
This revision was automatically updated to reflect the committed changes.
whchung marked an inline comment as done.
mlir/lib/Conversion/GPUToCUDA/CMakeLists.txt
5

This seems to break the build, SOURCES can be empty when I try to build it.

whchung marked an inline comment as done.May 20 2020, 4:18 PM
whchung added inline comments.
mlir/lib/Conversion/GPUToCUDA/CMakeLists.txt
5

@nicolasvasilache in your setup, MLIR_CUDA_CONVERSIONS_ENABLED is OFF, correct? Let me see how to address this.

@nicolasvasilache could you check if D80343 solves your issue? Sorry for the confusion.

rsmith added a subscriber: rsmith.May 20 2020, 6:54 PM
rsmith added inline comments.
mlir/tools/mlir-cuda-runner/cuda-runtime-wrappers.cpp
39–40

I'm seeing test failures caused by undefined references to mgpuModuleGetFunction. Did you intend to rename this function too?

I revert this change because the bot was broken, cmake fails when -DLLVM_TARGETS_TO_BUILD=host is passed in.

whchung reopened this revision.May 20 2020, 9:50 PM
whchung marked an inline comment as done.
This revision is now accepted and ready to land.May 20 2020, 9:50 PM
whchung updated this revision to Diff 265418.May 20 2020, 9:51 PM

Fix build errors. Adopting changes in D80343 and D80353.

Address review comments from @mehdi_amini in D80343.

This revision was automatically updated to reflect the committed changes.
rriddle added inline comments.May 27 2020, 1:56 PM
mlir/include/mlir/Conversion/GPUCommon/GPUCommonPass.h
12

Where is functional used?

14

Why are these necessary?

whchung marked 3 inline comments as done.May 27 2020, 2:25 PM
whchung added inline comments.
mlir/include/mlir/Conversion/GPUCommon/GPUCommonPass.h
14

These headers were introduced back in commit c72c6c390710 , but they don't really seem necessary now. I can modify D80142 and remove them.