Page MenuHomePhabricator

[MLIR][BufferPlacement] Support functions that return Memref typed results
ClosedPublic

Authored by dfki-ehna on May 28 2020, 12:43 AM.

Details

Summary

Buffer placement can now operates on functions that return buffers. These buffers escape from the deallocation phase of buffer placement.

Diff Detail

Event Timeline

dfki-ehna created this revision.May 28 2020, 12:43 AM
dfki-ehna added a project: Restricted Project.

Just some nits.

mlir/include/mlir/Transforms/BufferPlacement.h
109

Maybe rephrase in what it does rather than what it should have done.

Rewrites the ReturnOp to conform with the changed function signature. Operands that correspond to return values that have been rewritten from tensor results to memref arguments are dropped. In their place, a corresponding copy operation from the operand to the new function argument is inserted.

145

Nit: argument -> arguments

mlir/lib/Transforms/BufferPlacement.cpp
399

If it escapes, there should be no dealloc. Is that worth asserting? Can we have a case where such a dealloc exists? What should be done in such cases?

mlir/test/Transforms/buffer-placement.mlir
471

This is missing the CHECK lines.

mlir/test/lib/Transforms/TestBufferPlacement.cpp
53–55

ShapedType does not require rank. I do not understand this assertion. If it is not a ShapedType then there is simply nothing to do. As this is a test pass, I am fine with not supporting scalars, though. With a better assertion message.

89

Before adding the extra arguments, we know that oldBlock and newBlock have the same number of arguments here. So why not do this earlier and use the version of map with iterators instead of the loop? Something like mapping.map(oldBlock.getArguments(), newBlock->getArguments())

153

Is the WalkResult needed here or would implicit conversion just work?

dfki-ehna updated this revision to Diff 266806.May 28 2020, 4:03 AM

Addressed the comments.

dfki-ehna marked 11 inline comments as done.May 28 2020, 4:21 AM
dfki-ehna added inline comments.
mlir/lib/Transforms/BufferPlacement.cpp
399

Asserts now if there is any DeallocOp. The other possibility is erasing that DeallocOp instead of asserting it. If a DeallocOp exists before the last user (ReturnOp), then it is already in the input IR and it has not been created by the Buffer Placement. Although, In this case, the rewriter also produces an error that the value is not dominated.

mlir/test/lib/Transforms/TestBufferPlacement.cpp
53–55

The Assert message was changed.

89

Thanks.

153

Unfortunately, the implicit conversion produces a compile error.

herhut added inline comments.May 28 2020, 6:19 AM
mlir/test/lib/Transforms/TestBufferPlacement.cpp
76–77

Looking at this again, would it not be easier to use rewriter.inlineRegionBefore to move over the entire region into the new op and then only add the new arguments to the first block? Sorry for not suggesting this earlier.

153

Maybe making the return type of the lambda explicit would help? Like [&](FuncOp function) -> WalkResult {...?

dfki-ehna updated this revision to Diff 266870.May 28 2020, 7:19 AM
dfki-ehna marked 4 inline comments as done.

Change walk return type to WalkResult and move BlockAndValueMapping before adding new arguments to the new block.

dfki-ehna marked 3 inline comments as done.May 28 2020, 7:23 AM
dfki-ehna added inline comments.
mlir/test/lib/Transforms/TestBufferPlacement.cpp
76–77

Thanks for the suggestion. The problem in this way is that we should add the new arguments using block.addArgument which happens outside of the rewriter and is not undoable.

dfki-ehna marked 2 inline comments as done.May 28 2020, 7:24 AM
dfki-ehna added inline comments.
mlir/test/lib/Transforms/TestBufferPlacement.cpp
89

It also moved before adding the new arguments.

herhut accepted this revision.May 28 2020, 7:55 AM

Thanks!

This revision is now accepted and ready to land.May 28 2020, 7:55 AM
rriddle added inline comments.May 28 2020, 11:20 AM
mlir/test/lib/Transforms/TestBufferPlacement.cpp
95

This mapping happens automatically.

Removing unnecessary mapping.

dfki-ehna marked 2 inline comments as done.May 29 2020, 12:13 AM
dfki-ehna added inline comments.
mlir/test/lib/Transforms/TestBufferPlacement.cpp
95

Thanks.

This revision was automatically updated to reflect the committed changes.
dfki-ehna marked an inline comment as done.