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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Transforms/BufferPlacement.cpp | ||
---|---|---|
87 | Should we consider changing that interface? It should be easy for the consume to handle the case of no information at all. | |
294 | 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() ({ ^bb0: ... ^bb1: return_to_other_region() }, { ^bb2: ... } Then the flow goes from bb1 to bb2 but this code would register bb0 to bb1, no? | |
744 | nit coorresponding needs to shed an o | |
788 | Could this be a ValueRange? | |
789 | Could this be an OperandRange? | |
811 | Could this be an llvm:all_of? |
Completed support for loops using structured control flow and addressed reviewer comments.
Great to see the region branch op interface being used for this. Some minor comments.
mlir/lib/Transforms/BufferPlacement.cpp | ||
---|---|---|
84 | You are creating a null attribute here as opposed to an "empty" attribute. | |
550 | Assert that the find will not fail? | |
562 | Determine -> Check | |
563–565 | 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. | |
mlir/test/Transforms/buffer-placement.mlir | ||
1159–1164 | 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.) |
mlir/lib/Transforms/BufferPlacement.cpp | ||
---|---|---|
84 | I think that is what is required but I agree the comment is wrong. | |
87 | Yes, new PR sounds good to me. | |
267 | 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. | |
443 | This is not necessarily are block argument anymore. Also, maybe elaborate what the conditional means. | |
531 | nit: is belongs | |
559 | Which invariant does this break? | |
563–565 | Maybe make this a named lambda for improved readability? Filter successors that return to the parent operation. is what this lambda does, right? | |
572 | There is no operandProvider anymore. | |
616 | Which invariant is being broken here? |
Addressed reviewer comments regarding tests, comments and invariants.
mlir/lib/Transforms/BufferPlacement.cpp | ||
---|---|---|
267 | 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. |
Looks good with comments fixed. Thanks!
mlir/lib/Transforms/BufferPlacement.cpp | ||
---|---|---|
448 | 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. | |
561 | If the RegionSuccessor has not successor entry, it will return to the parent operation. |
mlir/lib/Transforms/BufferPlacement.cpp | ||
---|---|---|
81 | This looks like it's missing a static. |
This looks like it's missing a static.