This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add loop.parallel, loop.reduce and loop.reduce.return operations.
ClosedPublic

Authored by akuegel on Jan 8 2020, 5:13 AM.

Details

Summary

These operations can be used to specify a loop nest with a body that can contain reductions.
The iteration space can be iterated in any order.

RFC: https://groups.google.com/a/tensorflow.org/d/topic/mlir/pwtSgiKFPis/discussion

Diff Detail

Event Timeline

akuegel created this revision.Jan 8 2020, 5:13 AM

Unit tests: pass. 61132 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

akuegel updated this revision to Diff 236800.Jan 8 2020, 5:40 AM

Rebased to head.

Unit tests: pass. 61306 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

akuegel updated this revision to Diff 236818.Jan 8 2020, 7:02 AM

Improved description of parallel loop operation.

akuegel updated this revision to Diff 236822.Jan 8 2020, 7:10 AM

Also add description for loop.yield (thanks herhut@)

Unit tests: pass. 61312 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61312 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

rriddle requested changes to this revision.Jan 8 2020, 10:29 AM
rriddle added inline comments.
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
168

These should all be wrapped at 80 characters.

173

Keep these operations sorted alphabetically.

211

Use:

mlir
mlir/lib/Dialect/LoopOps/LoopOps.cpp
231

Drop the const &

nit: auto -> Value

233

Don't use -> or * with values.

246

nit: auto -> Block

251

Drop the &.

252

Same here, drop all of the -> and *

332

Stream in the results directly:
<< op.getResultTypes()

362

Drop extra newline

416

Stream the result and the type.

This revision now requires changes to proceed.Jan 8 2020, 10:29 AM
herhut resigned from this revision.Jan 9 2020, 1:34 AM

We pair programmed some of this code, so someone else needs to review. Obviously, I am happy for this to land :)

herhut added a subscriber: herhut.Jan 9 2020, 1:35 AM
akuegel updated this revision to Diff 237058.Jan 9 2020, 6:36 AM
akuegel marked 11 inline comments as done.

Addressed review comments.

akuegel updated this revision to Diff 237070.Jan 9 2020, 6:52 AM

Fix copy/paste issue.

Unit tests: pass. 61338 tests passed, 0 failed and 738 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61673 tests passed, 0 failed and 779 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

rriddle added inline comments.Jan 9 2020, 10:34 AM
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
171

Can you format this a bit better?

let arguments = (ins Variadic...,

Variadic...,
Variadic...);
290

This looks like it needs to be formatted.

297

nit: Can we name this something other than 'operation'? Seems like this is likely to lead to confusion in the future. e.g. computation or body would achieve the same effect.

mlir/lib/Dialect/LoopOps/LoopOps.cpp
240

I would imagine that we would check this before the above loop.

262

Use camelCase for variable names.

341

Drop the -> here.

342

I don't see a check for empty block, can you add one.

383

Drop -> here. Same in a few other places below.

mlir/include/mlir/Dialect/LoopOps/LoopOps.td
152

typo: individual
typo2: modeled

163

Please add a few more examples that also illustrate reduction and the yield ops.

261

Do we foresee another use case for this than reduction?
If not, does it make sense to call this a ReductionOp or a ReduceOp?

290

can we make this example a bit more realistic?
Unless I am misunderstanding, there should be a reduction operation in the region?

mlir/test/Dialect/Loops/invalid.mlir
130

Here and below: we usually omit the checks that are automatically tablegen'd.

akuegel updated this revision to Diff 237281.Jan 10 2020, 3:49 AM
akuegel marked 17 inline comments as done.

Address review comments.

akuegel added inline comments.Jan 10 2020, 3:51 AM
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
163

I extended the example so that it also has a loop.reduce inside.

171

I hope I understood your suggestion correctly. I have now indented it such that the words Variadic align.

261

I renamed it to ReduceOp, and accordingly the other op to ReduceReturnOp instead of YieldReturnOp.
Also, the textual format is now loop.reduce and loop.reduce.return.

290

My editor decided it should add a tab when I hit carriage return. Visually it looked good in my editor, but of course I should use spaces here for indentation :)

297

What about reductionOperator?

mlir/lib/Dialect/LoopOps/LoopOps.cpp
342

I think this is automatically checked because it is defined as a SizedRegion. See the test yield_no_block in invalid.mlir
But as far as I understand Nicolas' comment, I actually should remove that test, too (leaving it here for now for verification if maybe I misunderstood something).

Unit tests: pass. 61744 tests passed, 0 failed and 780 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

akuegel updated this revision to Diff 237282.Jan 10 2020, 4:12 AM

Ran git-clang-format.

Unit tests: pass. 61744 tests passed, 0 failed and 780 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

rriddle added inline comments.Jan 10 2020, 10:16 AM
mlir/lib/Dialect/LoopOps/LoopOps.cpp
332

nit: Remove trivial braces

342

SizedRegion means the number of blocks, it doesn't constrain the number of operations within the blocks. e.g. an empty block would crash the isa<ReduceReturnOp> check below.

359

Still seems like there is a newline here.

nicolasvasilache added a reviewer: andydavis1. nicolasvasilache removed 1 blocking reviewer(s): nicolasvasilache.Jan 10 2020, 3:55 PM

LGTM once River's comments are addressed, thanks a lot for pushing this!

akuegel updated this revision to Diff 237597.Jan 13 2020, 1:56 AM
akuegel marked 3 inline comments as done.

Addressed review comments.

akuegel added inline comments.Jan 13 2020, 1:57 AM
mlir/lib/Dialect/LoopOps/LoopOps.cpp
342

Ah, I mixed up those two. Thanks for explaining :)
I have now added a check for this as well, and modified the test to not test for no block, but instead for whether the block is empty.

Unit tests: pass. 61776 tests passed, 0 failed and 780 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

rriddle accepted this revision.Jan 13 2020, 9:55 AM
rriddle added inline comments.
mlir/lib/Dialect/LoopOps/LoopOps.cpp
348

nit: You can use emitOpError to have the message prefixed with the operation name: 'foo.op' op <your-message>

353

nit: use lower case to start the message

akuegel updated this revision to Diff 237883.Jan 14 2020, 1:34 AM
akuegel marked 2 inline comments as done.

Addressed River's review comments.

Unit tests: pass. 61801 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

akuegel retitled this revision from [mlir] Add skeleton implementation of loop.parallel, loop.yield and loop.yield.return operations. to [mlir] Add loop.parallel, loop.reduce and loop.reduce.return operations..Jan 14 2020, 1:57 AM
akuegel edited the summary of this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Jan 14 2020, 2:37 AM
This revision was automatically updated to reflect the committed changes.

I fixed the clang-format error in a followup commit.
Sorry for not noticing it sooner :-(