Page MenuHomePhabricator

[mlir] Added support for loops in BufferPlacement transformation.

Authored by dfki-mako on Aug 7 2020, 3:39 AM.



The current BufferPlacement transformation cannot handle loops properly. Buffers passed via backedges will not be freed automatically introducing memory leaks. This CL adds support for loops to overcome these limitations.

Diff Detail

Event Timeline

dfki-mako created this revision.Aug 7 2020, 3:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2020, 3:39 AM
dfki-mako requested review of this revision.Aug 7 2020, 3:39 AM
dfki-mako retitled this revision from [mlir] Added support for loops in BufferPlacement transformation. to [mlir] WIP: Added support for loops in BufferPlacement transformation..Aug 7 2020, 3:39 AM
dfki-mako added reviewers: herhut, pifon2a.
herhut added inline comments.Aug 10 2020, 3:50 AM

Should we consider changing that interface? It should be easy for the consume to handle the case of no information at all.


This assumes that it is always the first block in the region, entryBlock, that goes to the region successors. However, it could also be any other block in the region that has cross-region flow. For example

op_with_regions() ({


}, {



Then the flow goes from bb1 to bb2 but this code would register bb0 to bb1, no?


nit coorresponding needs to shed an o


Could this be a ValueRange?


Could this be an OperandRange?


Could this be an llvm:all_of?

dfki-mako updated this revision to Diff 287605.Aug 25 2020, 2:53 AM
dfki-mako marked 6 inline comments as done.

Completed support for loops using structured control flow and addressed reviewer comments.

dfki-mako added inline comments.Sep 2 2020, 3:46 AM

I guess we should provide the possibility to query the interface without providing an initialized operand-attributes list. However, we should address this in another PR. What do you think?


Yes, you are right. This issue has been resolved.

dfki-mako retitled this revision from [mlir] WIP: Added support for loops in BufferPlacement transformation. to [mlir] Added support for loops in BufferPlacement transformation..Sep 2 2020, 3:46 AM

Great to see the region branch op interface being used for this. Some minor comments.


You are creating a null attribute here as opposed to an "empty" attribute.


Assert that the find will not fail?


Determine -> Check


This comment looks confusing. If the successor is the parent operation, this returns nullptr. So there is no successor entry when returning to the parent op.


It may good to indent the first test case so that it's more readable, and to also add in the alloc, cmpi, and closing brace. (No need to capture/match SSA values for them though.)

herhut added inline comments.Sep 7 2020, 3:47 AM

I think that is what is required but I agree the comment is wrong.


Yes, new PR sounds good to me.


Should this query the RegionBranchOpInterface so that it only recurses into actual entry regions? For the current use case (asserting there are no cycles in the CFG) this is probably OK but it still is not correct.

Also, this is not tested, at all.


This is not necessarily are block argument anymore.

Also, maybe elaborate what the conditional means.


nit: is belongs


Which invariant does this break?


Maybe make this a named lambda for improved readability?

Filter successors that return to the parent operation. is what this lambda does, right?


There is no operandProvider anymore.


Which invariant is being broken here?

dfki-mako updated this revision to Diff 290433.Sep 8 2020, 2:06 AM
dfki-mako marked 14 inline comments as done.

Addressed reviewer comments regarding tests, comments and invariants.


I guess querying the RegionBranchOpInterface is not required to detect explicit control-flow loops. Furthermore, these loop can occur in any region not only the entry regions. What do you think?

Regarding testing: I have added additional test cases to trigger the internal Backedges analysis.

herhut accepted this revision.Sep 8 2020, 3:25 AM

Looks good with comments fixed. Thanks!


So this is spelling out dominance. A value is dominated by another value of their corresponding blocks dominate or if the value is before the other value if they are in the same block.

I do not see the implicitly. It dominates all operations in the block, which also means it will dominate the inserted alloc.

So this is correct, but the comment is a little misleading when reading it in the future I think.


If the RegionSuccessor has not successor entry, it will return to the parent operation.

This revision is now accepted and ready to land.Sep 8 2020, 3:25 AM
rriddle added inline comments.Sep 8 2020, 3:27 AM

This looks like it's missing a static.

This revision was automatically updated to reflect the committed changes.
dfki-mako marked 3 inline comments as done.