This is based on the use of code constantly checking for an attribute on
a model and instead represents the distinct operaion with a different
op. Instead, this op can be used to provide better filtering.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Cool stuff! :)
mlir/include/mlir/Conversion/GPUToCUDA/GPUToCUDAPass.h | ||
---|---|---|
22 | Nit: move this after template <typename OpT> class OpPassBase to keep fwd declared symbols in mlir together? Same for the following file. | |
mlir/include/mlir/Dialect/GPU/GPUOps.td | ||
594 | We can directly use IsolatedFromAbove here. | |
598 | 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. | |
601 | 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? | |
609 | The code should start a new line. | |
615 | Should we skipDefaultBuilders given that the default generated ones cannot guarantee terminating with gpu.module_end? | |
mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp | ||
67 | KernelGPU seems saying the same thing with different word... What about either KernelModule or GPUModule? | |
316 | Can we wrap this in namespace { ... }? | |
mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.td | ||
10 | Nit: SPIR-V |
Thanks for adding more structure! This needs a little more work on the description but otherwise looks good to me.
mlir/include/mlir/Dialect/GPU/GPUOps.td | ||
---|---|---|
601 | 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. | |
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp | ||
830 | 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? |
Rename GpuModule to GPUModule and improve description.
This was responding to various comments on the code review.
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp | ||
---|---|---|
830 | We need GPUModuleOp::build because we have to ensure a terminator, we can't use a generated create method as I understand it. The create method is to be consistent with similar ops like those that Stephan referenced. |
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp | ||
---|---|---|
830 | ModuleOp has a create because it is the top level operation(also partially legacy), FuncOp only has one because it was a part of the transition from separate class to operation that was never removed. Realistically, I don't see why GPUModuleOp needs one given that it is not any more difficult for a user to just have: OpBuilder builder(context); |
SPIR-V side LGTM. I'd wait @herhut to give the final approval.
mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.td | ||
---|---|---|
4 | Only need to change the first one. See an example here: https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/ADT/TypeSwitch.h |
Remove GpuModuleOp::create.
This is because GPUModuleOp is inside of another Module always, so it does not
need a static create method and can use OpBuilders.
LGTM from my side if rriddle@ is happy with the latest changes.
mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp | ||
---|---|---|
186–189 | Can you use kernelFunc.getLoc() here? | |
188–191 | Maybe SymbolTable should also provide a create<T> abstraction to hide these details of construction an operation. Not for this change, though. |
mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp | ||
---|---|---|
188–191 | That would be fine to add in a followup. |
Nit: move this after template <typename OpT> class OpPassBase to keep fwd declared symbols in mlir together? Same for the following file.