This is an archive of the discontinued LLVM Phabricator instance.

[mlir][gpu] Add customer printer/parser for gpu.launch_func.
ClosedPublic

Authored by csigg on Oct 12 2020, 12:59 PM.

Diff Detail

Event Timeline

csigg created this revision.Oct 12 2020, 12:59 PM
csigg requested review of this revision.Oct 12 2020, 12:59 PM
csigg updated this revision to Diff 297781.Oct 13 2020, 1:05 AM

Add space after 'in'.

herhut added a subscriber: ftynse.Oct 13 2020, 1:15 AM

I like this updated syntax. @ftynse WDYT?

mlir/include/mlir/Dialect/GPU/GPUOps.td
410

The custom<Space> is ugly but I see its motivation. Maybe file a bug to add support for explicit whitespace to the assembly format?

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

I'd prefer to expose the parsing of just the argument list instead of parsing a result type and then failing.

csigg updated this revision to Diff 297788.Oct 13 2020, 1:57 AM

Add test for custom formatter without arguments.

csigg updated this revision to Diff 297841.Oct 13 2020, 6:18 AM

Fix tests, expose just parsing function arguments.

csigg marked an inline comment as done.Oct 13 2020, 7:09 AM
csigg added inline comments.
mlir/include/mlir/Dialect/GPU/GPUOps.td
410

Alternatively I could custom-parse/print the grid in or even with the operands. That would look less ugly, but it's a little less expressive.

csigg retitled this revision from Add customer printer/parser for gpu.launch_func. to [mlir][gpu] Add customer printer/parser for gpu.launch_func..Oct 13 2020, 8:37 AM
csigg updated this revision to Diff 297881.Oct 13 2020, 9:08 AM

Update gpu.launch_func documentation.

csigg updated this revision to Diff 298931.Oct 18 2020, 10:38 PM

Use space literal in assemblyFormat.

csigg added inline comments.Oct 18 2020, 10:39 PM
mlir/include/mlir/Dialect/GPU/GPUOps.td
410

Switched to space literal.

ftynse added inline comments.Oct 19 2020, 12:44 AM
mlir/include/mlir/Dialect/GPU/GPUOps.td
21

Are we sure this isn't necessary anymore?

366

Syntax nit: I'd consider having having an prefix here too, something like args.

csigg added inline comments.Oct 19 2020, 6:10 AM
mlir/include/mlir/Dialect/GPU/GPUOps.td
21

I don't think so, because we lower from launch_func to runtime calls in one go.

366

Sure, I can do that.

csigg updated this revision to Diff 299102.Oct 19 2020, 10:39 AM

Add args keyword before argument list.
Change grid/block to blocks/threads.

Thanks. Just some nits.

mlir/include/mlir/Dialect/GPU/GPUOps.td
366

Can you update the example, too?

mlir/include/mlir/IR/FunctionImplementation.h
61 ↗(On Diff #299102)

You could add an option here whether it may have attributes, like allowAttributes or something and then parsing would just fail with a parse error.

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

Nit: This could be in the assemblyFormat

csigg updated this revision to Diff 299616.Oct 21 2020, 2:49 AM
csigg marked 2 inline comments as done.

Apply reviewer comments.

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

The args(...) is optional, and I don't think custom<> inside an optional group is supported. I left it as is.

csigg updated this revision to Diff 299642.Oct 21 2020, 5:09 AM

Rebase.

herhut accepted this revision.Oct 21 2020, 6:56 AM

Thanks!

This revision is now accepted and ready to land.Oct 21 2020, 6:56 AM