Page MenuHomePhabricator

[mlir] Add base class for GpuKernelToBlobPass

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



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

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


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


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

This is copying the string, why not StringRef?


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.

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.


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

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

(Did you miss this comment?)

csigg added inline comments.Mar 10 2021, 12:25 PM

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

Harbormaster completed remote builds in B93039: Diff 329571.