Page MenuHomePhabricator

[mlir] Add base class for GpuKernelToBlobPass
ClosedPublic

Authored by csigg on Mar 9 2021, 11:47 AM.

Details

Summary

Instead of configuring kernel-to-cubin/rocdl lowering through callbacks, introduce a base class that target-specific passes can derive from.

Put the base class in GPU/Transforms, according to the discussion in D98203.

The mlir-cuda-runner will go away shortly, and the mlir-rocdl-runner as well at some point. I therefore kept the existing code path working and will remove it in a separate step.

Depends On D98168

Diff Detail

Event Timeline

csigg created this revision.Mar 9 2021, 11:47 AM
csigg requested review of this revision.Mar 9 2021, 11:47 AM
csigg updated this revision to Diff 329544.Mar 9 2021, 10:58 PM

Add options for triple, chip, features.

csigg updated this revision to Diff 329545.Mar 9 2021, 11:02 PM

Remove accidental mlir-cuda-runner change.

herhut added inline comments.Mar 10 2021, 12:12 AM
mlir/include/mlir/Dialect/GPU/Passes.h
49

This is not public API but rather an implementation detail. Could it move to an implementation header in lib?

mlir/lib/Dialect/GPU/Transforms/SerializeToBlob.cpp
12

Not sure this comment is correct. It translates the entire module, no?

33

I have never seen a pass copy options before. How is this normally handled?

herhut accepted this revision.Mar 10 2021, 12:22 AM

After off-line discussion that this will be morphed into the final state in multiple changes, this looks like a good first step. Please fix the comment, though.

This revision is now accepted and ready to land.Mar 10 2021, 12:22 AM
rriddle added inline comments.Mar 10 2021, 12:33 AM
mlir/lib/Conversion/GPUCommon/ConvertKernelFuncToBlob.cpp
71

This is copying the string, why not StringRef?

mlir/lib/Dialect/GPU/Transforms/SerializeToBlob.cpp
33

Pass options are copied implicitly as part of clone, i.e. you shouldn't need to do anything.

csigg updated this revision to Diff 329561.Mar 10 2021, 12:36 AM

Fix file comment.

csigg marked an inline comment as done.Mar 10 2021, 12:36 AM
csigg added inline comments.
mlir/include/mlir/Dialect/GPU/Passes.h
49

At the moment it is public API because it's used outside.

Once we cleaned up the use in Conversion/GPUCommon/ConvertKernelFuncToBlob.cpp (which I will do in a separate CL), we can move it.

mlir/lib/Dialect/GPU/Transforms/SerializeToBlob.cpp
33

Pass::Option is currently missing a copy c'tor. I didn't want to delay this change (or make unrelated changes) and will clean it up separately.

csigg updated this revision to Diff 329564.Mar 10 2021, 12:52 AM

Change isa string to reference.

csigg added inline comments.Mar 10 2021, 12:54 AM
mlir/lib/Conversion/GPUCommon/ConvertKernelFuncToBlob.cpp
71

Good point. I changed it to a string const-reference because it is used as null-terminated char pointer.

csigg updated this revision to Diff 329569.Mar 10 2021, 1:23 AM

Reduce public #includes.

csigg updated this revision to Diff 329571.Mar 10 2021, 1:35 AM

Move LLVM dependencies.

This revision was landed with ongoing or failed builds.Mar 10 2021, 3:14 AM
This revision was automatically updated to reflect the committed changes.
rriddle added inline comments.Mar 10 2021, 10:28 AM
mlir/lib/Dialect/GPU/Transforms/SerializeToBlob.cpp
33

(Did you miss this comment?)

csigg added inline comments.Mar 10 2021, 12:25 PM
mlir/lib/Dialect/GPU/Transforms/SerializeToBlob.cpp
33

I did miss your comment, sorry. I will send a follow-up change.

Harbormaster completed remote builds in B93039: Diff 329571.