[PATCH] D72336: [mlir] Create a gpu.module operation for the GPU Dialect.

Wed Jan 8 02:24:57 PST 2020

herhut added a comment.

Thanks for adding more structure! This needs a little more work on the description but otherwise looks good to me.

================
Comment at: mlir/include/mlir/Dialect/GPU/GPUOps.td:597
+    single region with a single block. This block is expected to contain
+    operations that define a symbol. GPU modules are required to have a name
+    that is used for symbol resolution by the gpu.launch_func operation.
----------------
antiagainst wrote:
> It's a bit confusing for me here saying that `gpu.launch_func` is to launch a `gpu.module`... I'd expect a module to contain some entry functions that a `gpu.launch_func` can launch. Seems like we either need to have a `gpu.launch_module` to be consistent, which needs to define the semantics first for sure... Maybe consider revising the wording here at least?
I think the underlying problem is that we have no notion of qualified symbols implemented in MLIR yet. So the launch_func actually gets a module and function. I agree the description is misleading.

================
Comment at: mlir/lib/Dialect/GPU/IR/GPUDialect.cpp:822
+  OperationState state(loc, getOperationName());
+  Builder builder(loc->getContext());
+  GpuModuleOp::build(&builder, state, name);
----------------
rriddle wrote:
> Why is this method necessary?
It is the equivalent of ModuleOp::create and FuncOp::create to allow creation of modules outside the context of a surrounding module. We could refactor the code here to use a builder, as the module is inserted into a surrounding module but in general that might not be the case. Or is there a way to have create methods generated?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72336