[PATCH] D80142: [mlir][gpu][mlir-cuda-runner] Refactor ConvertKernelFuncToCubin to be generic.

[Stephan Herhut via Phabricator via llvm-commits]

herhut accepted this revision.
herhut added a comment.
This revision is now accepted and ready to land.

Thanks. I think this avoids a lot of code duplication. I would prefer to remove the initialize target callback and move that into the caller.

With that addressed I am happy to see this land.



================
Comment at: mlir/include/mlir/Conversion/GPUCommon/GPUCommonPass.h:39
+    std::function<OwnedBlob(const std::string &, Location, StringRef)>;
+using InitBackendCallback = std::function<LogicalResult()>;
+using LoweringCallback =
----------------
Do we need to actually do this in this pass or could we do this in the code that constructs the pipeline? If the calling context has to provide this callback, it could also just call the init itself.


================
Comment at: mlir/include/mlir/Conversion/GPUCommon/GPUCommonPass.h:40
+using InitBackendCallback = std::function<LogicalResult()>;
+using LoweringCallback =
+    std::function<LogicalResult(Operation *, std::unique_ptr<llvm::Module> &)>;
----------------
Could this be a `std::function<std::unique_ptr<llvm::Module>(Operation *)>` instead, signalling failure by returning a `nullptr`? 


================
Comment at: mlir/include/mlir/Conversion/GPUCommon/GPUCommonPass.h:56
+/// the 'gpu.kernel' attribute, copies it to a new LLVM module, compiles the
+/// module with help of the GPU backend to targte object and then invokes
+/// the provided blobGenerator to produce a binary blob. Such blob is then
----------------
Nit: `to target object`


================
Comment at: mlir/lib/Conversion/GPUCommon/ConvertKernelFuncToBlob.cpp:92
+
+  /// Converts llvmModule to a lob with target instructions using the
+  /// user-provided generator. Location is used for error reporting and name is
----------------
`lob` -> `blob`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80142/new/

https://reviews.llvm.org/D80142