Page MenuHomePhabricator
Differential D89262

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

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

Details

Reviewers
herhut
rriddle
Commits
rG1c1803dbb0f6: [mlir][gpu] Add customer printer/parser for gpu.launch_func.

Diff Detail

Unit TestsFailed
View All

TimeTest
40 mslinux > MLIR.Dialect/GPU::invalid.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/mlir-opt -split-input-file -verify-diagnostics /mnt/disks/ssd0/agent/llvm-project/mlir/test/Dialect/GPU/invalid.mlir
10 mslinux > MLIR.Dialect/GPU::ops.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/mlir-opt -allow-unregistered-dialect /mnt/disks/ssd0/agent/llvm-project/mlir/test/Dialect/GPU/ops.mlir | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/mlir/test/Dialect/GPU/ops.mlir
130 mslinux > MLIR.Dialect/GPU::outlining.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/mlir-opt -allow-unregistered-dialect -gpu-kernel-outlining -split-input-file -verify-diagnostics /mnt/disks/ssd0/agent/llvm-project/mlir/test/Dialect/GPU/outlining.mlir | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/mlir/test/Dialect/GPU/outlining.mlir
60 mswindows > MLIR.Dialect/GPU::invalid.mlir
Script: -- : 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\mlir-opt.exe -split-input-file -verify-diagnostics C:\ws\w16-1\llvm-project\premerge-checks\mlir\test\Dialect\GPU\invalid.mlir
50 mswindows > MLIR.Dialect/GPU::ops.mlir
Script: -- : 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\mlir-opt.exe -allow-unregistered-dialect C:\ws\w16-1\llvm-project\premerge-checks\mlir\test\Dialect\GPU\ops.mlir | c:\ws\w16-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16-1\llvm-project\premerge-checks\mlir\test\Dialect\GPU\ops.mlir
View Full Test Results (6 Failed)

Event Timeline

csigg created this revision.Oct 12 2020, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2020, 12:59 PM
Herald added subscribers: rdzhabarov, tatianashp, msifontes and 14 others. · View Herald Transcript
csigg requested review of this revision.Oct 12 2020, 12:59 PM
Herald added a reviewer: herhut. · View Herald TranscriptOct 12 2020, 12:59 PM
Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript
Harbormaster completed remote builds in B74843: Diff 297674.Oct 12 2020, 1:18 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.

Harbormaster completed remote builds in B74904: Diff 297781.Oct 13 2020, 1:21 AM
csigg updated this revision to Diff 297788.Oct 13 2020, 1:57 AM

Add test for custom formatter without arguments.

Harbormaster completed remote builds in B74908: Diff 297788.Oct 13 2020, 2:15 AM
csigg updated this revision to Diff 297841.Oct 13 2020, 6:18 AM

Fix tests, expose just parsing function arguments.

Herald added a reviewer: rriddle. · View Herald TranscriptOct 13 2020, 6:18 AM
Harbormaster completed remote builds in B74930: Diff 297841.Oct 13 2020, 6:33 AM
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.

Harbormaster completed remote builds in B74946: Diff 297881.Oct 13 2020, 9:25 AM
csigg added a child revision: D89324: [mlir][gpu] Allow gpu.launch_func to be async..Oct 13 2020, 12:02 PM
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.

Harbormaster completed remote builds in B75489: Diff 298931.Oct 18 2020, 10:45 PM
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.

Harbormaster completed remote builds in B75563: Diff 299102.Oct 19 2020, 10:58 AM
herhut added a comment.Oct 20 2020, 3:02 AM

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.

Harbormaster completed remote builds in B75845: Diff 299616.Oct 21 2020, 2:58 AM
csigg updated this revision to Diff 299642.Oct 21 2020, 5:09 AM

Rebase.

Harbormaster completed remote builds in B75861: Diff 299642.Oct 21 2020, 5:25 AM
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
Closed by commit rG1c1803dbb0f6: [mlir][gpu] Add customer printer/parser for gpu.launch_func. (authored by csigg). · Explain WhyOct 21 2020, 9:19 AM
This revision was automatically updated to reflect the committed changes.
csigg added a commit: rG1c1803dbb0f6: [mlir][gpu] Add customer printer/parser for gpu.launch_func..
csigg removed a child revision: D89324: [mlir][gpu] Allow gpu.launch_func to be async..Oct 21 2020, 10:53 PM

Revision Contents
Changeset List

PathSize
mlir/
include/
mlir/
Dialect/
GPU/
24 lines
lib/
Dialect/
GPU/
IR/
62 lines
test/
Dialect/
GPU/
61 lines
14 lines
16 lines
mlir-tblgen/
3 lines
tools/
mlir-tblgen/
19 lines

Diff 297788

View Options

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

Loading...
View Options

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

Loading...
View Options

mlir/test/Dialect/GPU/invalid.mlir

Loading...
View Options

mlir/test/Dialect/GPU/ops.mlir

Loading...
View Options

mlir/test/Dialect/GPU/outlining.mlir

Loading...
View Options

mlir/test/mlir-tblgen/op-format-spec.td

Loading...
View Options

mlir/tools/mlir-tblgen/OpFormatGen.cpp

Loading...