Page MenuHomePhabricator

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

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



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?


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.


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


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


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


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


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


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


for_each or drop the brackets.


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.




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




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


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.


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

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


nit: Use a lambda instead.


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


nit: Use the full name for the type.


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


Is this guaranteed to be a RegionBranchOpInterface?


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?



herhut added inline comments.Jun 22 2020, 1:02 AM

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

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.


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


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


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.


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

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.

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


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

nit: interface


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.


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.


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?


Maybe Wire flow between regions and from region exits.?


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


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.


Should this apply to the case above, as well?


Where is this conversion? Or is this comment off?


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


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


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.


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


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.


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.