This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Extended BufferPlacement to support nested region control flow.
ClosedPublic

Authored by dfki-mako on Jun 16 2020, 4:11 AM.

Details

Summary

The current BufferPlacement implementation does not support nested region control flow. This CL adds support for nested regions via the RegionBranchOpInterface and the detection of branch-like (ReturnLike) terminators inside nested regions.

Diff Detail

Event Timeline

dfki-mako created this revision.Jun 16 2020, 4:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2020, 4:11 AM
herhut requested changes to this revision.Jun 16 2020, 5:04 AM

Can you also add a test with deeper nesting?

mlir/lib/Transforms/BufferPlacement.cpp
81

Another way to describe exitParentRegion would be to only consider immediately nested regions. You can use the approach you have now, or simply use a loop over regions/blocks and get their terminators in the true case. Less traversal work.

87

would terminator->getParentRegion()->getParentOp() == operation do the same?

136–195

detailed information is not very helpful in a comment. What does it query?

150

Maybe llvm::for_each? Drop the {,}.

207

So these are the entry regions for this operation. Maybe write that in the comment.

211

This ties the operands of the op with a region to the block arguments of the target region,

212

Block &successorBlock = regionSuccessor.getSuccessor()->front(); ?

213

for_each or drop the brackets.

217

Now that you have queried the flow-in, you can also query the flow within the op. For this, you can call regionInterface.getSuccessorRegions once for each region of the op. The alias registration is the same as with the initial flow into the op.

Except, when regionSuccessor.getSuccessor() is nulltptr. That signals that the terminator of the region will exit the region. So you have to tie the regionSuccessor.getSuccessorInputs() to parentOp->getResults() in that case.

221

/*exitParentRegion=*/false

221

I don't think this is needed with the above implemented.

296

front()?

443

This copies a region result, so maybe reflect that in the name.

451

This also needs to use the RegionBranchOpInterface to find all the terminators in the operation and their successor inputs (they might be in a different order than the op results). Querying the terminators directly makes assumptions about ordering that do not exist.

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

There should be a dealloc here, right?

This revision now requires changes to proceed.Jun 16 2020, 5:04 AM
dfki-mako retitled this revision from [mlir] Extended BufferPlacement to support nested region control flow. to [mlir] WIP: Extended BufferPlacement to support nested region control flow..Jun 17 2020, 4:16 AM
dfki-mako updated this revision to Diff 272028.Jun 19 2020, 5:26 AM
dfki-mako marked 15 inline comments as done.

Added extended support for the RegionOpInterface to query successor bindings for successor regions

rriddle added inline comments.Jun 19 2020, 11:05 AM
mlir/lib/Transforms/BufferPlacement.cpp
81

Please move static functions to the global namespace. anonymous namespace should only really be used by things like classes.

139

nit: Use a lambda instead.

154

nit: I don't think this really saves anything over a normal for loop, if anything it is much less efficient.

163

nit: Use the full name for the type.

188

nit: Use parameter names when passing constants, i.e., /*someName=*/

422

Is this guaranteed to be a RegionBranchOpInterface?

434

You are completely disregarding the fact that getMutableSuccessorOperands can return None, which would cause this to crash. Where are you checking that this is valid?

440

llvm::find?

herhut added inline comments.Jun 22 2020, 1:02 AM
mlir/lib/Transforms/BufferPlacement.cpp
163

Why is this called operands? These are the results of the overall operation, right?

165
This region does not have a valid successor block if it terminates the parent operation. In that case, we wire its successor operands with the parent operations results.

This could be more obvious in the code if you would pass the parent operation. Please at least rename operands to results.

170

The loop and the zip could be part of the helper function registerAliasFunc.

175

Isn't this the wrong way round? The results alias the successor inputs?

201

This only follows one level of branching. So if the entry region goes to region 2, and that one goes to region 3, the second link will not be seen.

It should be good enough to loop over all regions of an operation and then do this linking to successor regions.

434

The underlying assumption is that if the successor has block arguments, then the branch in the predecessor needs to have operands for those. Can we rely on this?

rriddle added inline comments.Jun 22 2020, 1:46 AM
mlir/lib/Transforms/BufferPlacement.cpp
434

Branching operations are not required to have a Value already materialized for a block argument. There are certain classes of operations that internally generate the value that is passed to the branch. For example, operations like LLVM Callbr and certain SIL switches.

dfki-mako updated this revision to Diff 272697.Jun 23 2020, 6:02 AM
dfki-mako marked 10 inline comments as done.

Refactored implementation and simplified the iteration over all successor regions.

dfki-mako marked 6 inline comments as done.Jun 23 2020, 6:06 AM
dfki-mako added inline comments.
mlir/lib/Transforms/BufferPlacement.cpp
422

The value should either be a BlockArgument or a value resulting from a RegionBranchOpInterface operation.

434

We have changed the code to emit an error message. We prefer the error message over silently ignoring this case as the retrieved alias information can be invalid if an operation passes values implicitly to a block argument.

dfki-mako retitled this revision from [mlir] WIP: Extended BufferPlacement to support nested region control flow. to [mlir] Extended BufferPlacement to support nested region control flow..Jun 24 2020, 3:51 AM
herhut added inline comments.Jun 24 2020, 4:00 AM
mlir/lib/Transforms/BufferPlacement.cpp
150

nit: interface

177

entryRegion.getSuccessorInputs returns the inputs of the target region, typically the block arguments of the first block. So this maps them with itself.
Instead, this should map the result from regionInterface().getSuccessorEntryOperands to the entryRegion.getSuccessorInputs.

193

The terminator should also implement the BranchOpInterface, right? So, one should query the inputs to the successor using BranchOpInterface.getSuccessorOperands, correct?

Here, successorRegion.getSuccessorInputs returns the input values to the region, which normally would be the block arguments or, in case this leaves the operation, the results.

dfki-mako updated this revision to Diff 273290.Jun 25 2020, 3:40 AM
dfki-mako marked 7 inline comments as done.

Added support for region-region control flow within operations that implement the RegionBranchOpInterface.
Added new test operations to verify more advanced region-region control-flow scenarios.

herhut requested changes to this revision.Jun 25 2020, 12:14 PM

Cool stuff. Great to see this work with complex region interfaces, as well!

So, this now assumes that there is a 1:1 mapping between return-like op operands and successor inputs and we likely want an interface that makes this configurable. Maybe the returnlike trait could provide a function that enables this. (We discussed this, just writing it down.)

Also, we need a clean-up pass to remove unneeded alloc+copy+dealloc triples. This is underway.

With the nits addressed and some of the comments cleaned up, this is good to go.

mlir/lib/Transforms/BufferPlacement.cpp
178

Mega-nit: entryRegion->getSuccessor reads weird. It read like getting the successor of the entry region but actually it gets the entry region itself :)

Maybe rename this entrySuccessor?

182

Maybe Wire flow between regions and from region exits.?

188

The length of operandAttributes might be wrong here, as it was built for the entry successors.

190

I think this comment is now off. It always wires the terminator operands with the successor inputs. The latter can be block arguments of a region's entry block or the result values, if the terminator exists the op.

295

Should this apply to the case above, as well?

397

Where is this conversion? Or is this comment off?

402

Nit: Doesn't this create a new alloc and a copy? If so, please fix the comment.

433

So is it OK to pass an empty ArrayRef here? If so, why can we not do this in the other cases?

mlir/test/lib/Dialect/Test/TestOps.td
1344 ↗(On Diff #273290)

Please use

let description = [{
...
}];

instead of comment.

This revision now requires changes to proceed.Jun 25 2020, 12:14 PM
herhut accepted this revision.Jun 26 2020, 4:03 AM

With my comments addressed, this is good to go.

This revision is now accepted and ready to land.Jun 26 2020, 4:03 AM
dfki-mako updated this revision to Diff 273669.Jun 26 2020, 4:51 AM
dfki-mako marked 10 inline comments as done.

Addressed reviewer comments.

mlir/lib/Transforms/BufferPlacement.cpp
177

Yeah makes sense to me; we should wire getSuccessorEntryOperands with getSuccessorInputs.

188

According to our interpretation of the comment

operands` is a set of optional attributes that correspond to a constant value for each operand

this should be an array containing values for each operand of the defining operation; although it might make sense to include an attribute for each block argument. However, we should keep this for now, as it is fully compatible with the current implementation of all instantiations of the RegionBrachOpInterface.

193

I guess querying a BranchOpInterface might not make sense in the case of a ReturnLike op, since it does not branch to any block in general (although this can still be expressed via the BranchOpInterface). It feels like that a ReturnLike terminator should provide a method to access all operands similar to the BranchOpInterface.

This revision was automatically updated to reflect the committed changes.