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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst requested changes to this revision.
antiagainst added a comment.
This revision now requires changes to proceed.

Cool stuff! :)



================
Comment at: mlir/include/mlir/Conversion/GPUToCUDA/GPUToCUDAPass.h:22
 
+namespace gpu {
+class GpuModuleOp;
----------------
Nit: move this after `template <typename OpT> class OpPassBase` to keep fwd declared symbols in `mlir` together? Same for the following file.


================
Comment at: mlir/include/mlir/Dialect/GPU/GPUOps.td:590
+def GPU_GpuModuleOp : GPU_Op<"module", [
+  NativeOpTrait<"IsIsolatedFromAbove">, SymbolTable, Symbol,
+  SingleBlockImplicitTerminator<"ModuleEndOp">
----------------
We can directly use `IsolatedFromAbove` here.


================
Comment at: mlir/include/mlir/Dialect/GPU/GPUOps.td:594
+  let summary = "A top level compilation unit to be run on a GPU.";
+  let description = [{
+    This op defines a GPU module. The module's top-level scope is modeled by a
----------------
It would be nice to be more clear regarding the purpose of this module, how it involves the host/device modelling, what's the invariants before jumping to the structural representation.


================
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.
----------------
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?


================
Comment at: mlir/include/mlir/Dialect/GPU/GPUOps.td:605
+
+    ``` gpu.module @symbol_name {
+        ...
----------------
The code should start a new line.


================
Comment at: mlir/include/mlir/Dialect/GPU/GPUOps.td:611
+    ```
+  }];
+  let builders = [OpBuilder<"Builder *builder, OperationState &result, "
----------------
Should we skipDefaultBuilders given that the default generated ones cannot guarantee terminating with gpu.module_end?


================
Comment at: mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp:67
+/// Pattern to convert a gpu.module to a spv.module.
+class KernelGpuModuleConversion final
+    : public SPIRVOpLowering<gpu::GpuModuleOp> {
----------------
`KernelGPU` seems saying the same thing with different word... What about either `KernelModule` or `GPUModule`?


================
Comment at: mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp:317
 
+#include "GPUToSPIRV.cpp.inc"
+
----------------
Can we wrap this in `namespace { ... }`?


================
Comment at: mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.td:9
+//
+// This file contains patterns to lower GPU dialect ops to to SPIRV ops.
+//
----------------
Nit: SPIR-V


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72336