[PATCH] D79850: [mlir] Extended BufferPlacement to support more sophisticated scenarios in which allocations cannot be moved freely and can remain in divergent control flow.
Stephan Herhut via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 14 08:38:26 PDT 2020
herhut requested changes to this revision.
herhut added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: jurahul.
Can you add tests for dynamic allocation cases, as well?
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:11
+// Furthermore, buffer placement also adds required new alloc and copy
+// operations to ensure that all buffers The main class is the
+// BufferPlacementPass class that implements the underlying algorithm. In order
----------------
Incomplete sentence `to ensure that all buffers`
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:37
+// post dominator. However, this requires a new copy buffer for %arg1 that will
+// contain the actual contents. Using the class BufferPlacementAliasAnalysis, we
+// will find out that %new_value has a potential alias %arg1. In order to find
----------------
In other words, the analysis uses the post-dominator to free the allocated memory. If no such post-dominator exists, aliases are removed by inserting allocs and copies. Is that the idea?
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:45
+// liveness information to determine the exact operation after which we have to
+// insert the dealloc. Finding the alloc position is highly similar and non-
+// obvious. However, the algorithm supports moving allocs to other places and
----------------
Nit: Remove `highly`.
So alloc works the same way but this time with a common dominator? If no such dominator exists, copies are inserted?
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:167
+ initBlockMapping();
+ // TODO: optimize allocations (fuse, move and reuse buffers).
+ }
----------------
Optimizations should be independent of `BufferPlacement`, so this TODO does not belong here.
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:186
+ void initBlockMapping() {
+ BufferPlacementAliasAnalysis aliases(operation);
+ operation->walk([&](MemoryEffectOpInterface opInterface) {
----------------
Would it make sense to compute this once and share it between the different phases? The moving of allocs does not influence the aliasing.
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:203
+ // (possibly existing) deallocation ops.
+ assert(allocateResultEffects.size() == 1 &&
+ "multiple allocations are not allowed");
----------------
This should not be an assert. Instead either ignore the `alloc` (which AFAIK was the mode before) or fail the transformation and report an error.
Generally, the reason this was not supported before was that multiple results creates situations where the alloc cannot be moved. Can the new system not tolerate this with copies?
I am OK with this not being supported, as a multi-result `alloc` seems a very boutique use.
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:217
+ BufferPlacementAliasAnalysis aliasAnalysis) {
+ // Check whether this is a known allocation node.
+ // TODO: remove this dependency by introducing a generic interface.
----------------
Allocation node with know shape? Another way to look at it would be to check whether the allocation has any operands, which structurally is the reason it cannot be moved.
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:227
+ // to a common post dominator in which all values are available.
+ BufferPlacementAliasAnalysis::ValueSetT dependencies(
+ ++dynamicSizes.begin(), dynamicSizes.end());
----------------
Same here, this could just use the operands of the alloc.
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:229
+ ++dynamicSizes.begin(), dynamicSizes.end());
+ return findPlacementBlock(*dynamicSizes.begin(), dependencies,
+ postDominators);
----------------
It is a clever reuse of `findPlacementBlock`. Maybe it should no longer talk about aliases. Instead, it is a helper that finds a dominator/postdominator for a range of values.
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:233
-public:
- BufferPlacementAnalysis(Operation *op)
- : operation(op), liveness(op), dominators(op), postDominators(op),
- aliases(op) {}
-
- /// Computes the actual positions to place allocs and deallocs for the given
- /// value.
- BufferPlacementPositions
- computeAllocAndDeallocPositions(OpResult result) const {
- if (result.use_empty())
- return BufferPlacementPositions(result.getOwner(), result.getOwner());
- // Get all possible aliases.
- auto possibleValues = aliases.resolve(result);
- return BufferPlacementPositions(getAllocPosition(result, possibleValues),
- getDeallocPosition(result, possibleValues));
+ // We do not have access to detailed dependency information. Hence, we place
+ // the alloc operation in the same block to ensure a valid program.
----------------
Is this `dependency information` that you are lacking?
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:281
+ for (Value value : it->second) {
+ auto blockArg = value.cast<BlockArgument>();
+ if (blockArgsToFree.count(blockArg) < 1 &&
----------------
Would it make sense to store `BlockArgument` to start with? Aliases are always `BlockArguments`, right?
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:284
+ !dominators.dominates(definingBlock, blockArg.getOwner()))
+ blockArgsToFree.insert(blockArg);
+ }
----------------
This breaks the aliasing between `source` and `value`. If there are further aliases of `source` that alias via `value`, they would no longer be an alias. But we would still potentially insert a copy to break the aliasing?
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:293
+ // Detect possibly unsafe aliases starting from all block arguments.
for (Region ®ion : operation->getRegions())
for (Block &block : region)
----------------
This does not account for nested blocks in nested regions. Also, why is this needed?
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:313
+ .slice(blockArg.getArgNumber(), 1);
+ Value sourceValue = ((OperandRange)successorOperand)[0];
+
----------------
Somewhat sad that `MutableOperandRange` does not support to get values out...
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:388
+ auto nextOp = endOperation->getNextNode();
+ endOperation = nextOp ? nextOp : endOperation;
+ OpBuilder builder(endOperation);
----------------
Why would we want to insert before the end operation? That would free it before the last use?
================
Comment at: mlir/lib/Transforms/BufferPlacement.cpp:446
+ getFunction().walk([&](MemoryEffectOpInterface opInterface) {
+ // Try to find a free effect that is applied to one of our values or a an
+ // involved block argument that will be automatically freed by our pass.
----------------
`a an` -> `an`
================
Comment at: mlir/test/Transforms/buffer-placement.mlir:87
+// CHECK-NEXT: br ^bb3
+// CHECK-NEXT: ^bb3(%[[ALLOC02:.*]]:{{.*}})
+// CHECK: linalg.copy(%[[ALLOC02]],
----------------
The numbering is strange of `ALLOC02`
================
Comment at: mlir/test/Transforms/buffer-placement.mlir:211
// CHECK-NEXT: %[[FIRST_ALLOC:.*]] = alloc()
// CHECK-NEXT: linalg.generic
----------------
I cannot follow this without the block numbers in the checks. Why do we get copies here?
================
Comment at: mlir/test/Transforms/buffer-placement.mlir:309
// CHECK-NEXT: linalg.generic
+// CHECK: %[[COPY_ALLOC0:.*]] = alloc()
+// CHECK-NEXT: linalg.copy({{.*}}, %[[COPY_ALLOC0]])
----------------
Same here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79850/new/
https://reviews.llvm.org/D79850
More information about the llvm-commits
mailing list