This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Create a gpu.module operation for the GPU Dialect.
ClosedPublic

Authored by tpopp on Jan 7 2020, 7:40 AM.

Details

Summary

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.

Diff Detail

Event Timeline

tpopp created this revision.Jan 7 2020, 7:40 AM
antiagainst requested changes to this revision.Jan 7 2020, 5:44 PM

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

This revision now requires changes to proceed.Jan 7 2020, 5:44 PM
rriddle retitled this revision from Create a gpu.module operation for the GPU Dialect. to [mlir] Create a gpu.module operation for the GPU Dialect..Jan 7 2020, 5:56 PM
rriddle requested changes to this revision.Jan 7 2020, 5:59 PM
rriddle added inline comments.
mlir/include/mlir/Dialect/GPU/GPUOps.td
591

Why are there extra newlines here?

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.td
4

LLVM -> MLIR

mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
821

nit: GpuModuleOp -> GPUModuleOp

830

Why is this method necessary?

858

nit: Merge this with the line above.

herhut added a comment.Jan 8 2020, 2:24 AM

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?

tpopp updated this revision to Diff 236979.Jan 9 2020, 1:28 AM
tpopp marked 15 inline comments as done.

Rename GpuModule to GPUModule and improve description.

This was responding to various comments on the code review.

tpopp added inline comments.Jan 9 2020, 1:48 AM
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.

rriddle added inline comments.Jan 9 2020, 10:43 AM
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);
builder.create<GPUModuleOp>(...);

antiagainst accepted this revision.Jan 11 2020, 5:27 AM

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

tpopp updated this revision to Diff 237605.Jan 13 2020, 2:34 AM
tpopp marked 5 inline comments as done.

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.

herhut accepted this revision.Jan 13 2020, 7:04 AM

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.

rriddle accepted this revision.Jan 13 2020, 10:38 AM
rriddle added inline comments.
mlir/lib/Dialect/GPU/Transforms/KernelOutlining.cpp
188–191

That would be fine to add in a followup.

This revision is now accepted and ready to land.Jan 13 2020, 10:38 AM
tpopp updated this revision to Diff 237879.Jan 14 2020, 1:21 AM

Change which location information is used at one point.

tpopp marked 3 inline comments as done.Jan 14 2020, 1:29 AM
This revision was automatically updated to reflect the committed changes.