This is an archive of the discontinued LLVM Phabricator instance.

[mlir][bufferization] Change semantics of DeallocOp result values
ClosedPublic

Authored by maerhart on Aug 3 2023, 5:19 AM.

Details

Summary

This change allows supporting operations for which we don't get precise aliasing information without the need to insert clone operations. E.g., arith.select.

Diff Detail

Event Timeline

maerhart created this revision.Aug 3 2023, 5:19 AM
Herald added a project: Restricted Project. · View Herald Transcript
maerhart requested review of this revision.Aug 3 2023, 5:19 AM
maerhart added inline comments.Aug 3 2023, 7:28 AM
mlir/lib/Dialect/Bufferization/IR/BufferizationOps.cpp
852

i should not be incremented when we see a duplicate.

maerhart added inline comments.Aug 3 2023, 7:41 AM
mlir/lib/Dialect/Bufferization/IR/BufferizationOps.cpp
886

Since the memref that is removed here is guaranteed to alias with the one in the retained list, we need to replace the uses of %0 with arith.ori %arg1, %0.

springerm added inline comments.Aug 3 2023, 7:42 AM
mlir/include/mlir/Dialect/Bufferization/IR/BufferizationOps.td
487–488

Let's add a paragraph here: The number of variadic memrefs` operands must equal the number of variadic conditions operands.`

488–512

Let's break it up a bit. Something along the lines of:

This operation returns a variadic number of  `updatedConditions` operands, one updated condition per retained memref. An updated condition indicates the ownership of the respective retained memref. It computed as the disjunction of all `conditions` operands where the corresponding to `memrefs` operand aliases with the retained memref. If the retained memref has no aliases among `memrefs`, the resulting updated condition is 'false' (TODO: explain why).
491

Dot missing

514–515

Let's add another memref to the retain list to show that the number of retained memrefs does not necessarily have to be 2.

mlir/lib/Conversion/BufferizationToMemRef/BufferizationToMemRef.cpp
93–94

add: without any retained values and a single memref

111–112

Let's put an assert(op.getRetained().empty() && "expected no retained memrefs"); just to be safe. Same for "one memref".

123–124

it

123–124

memref helper structure

124–129

replace with: lists (represented as memrefs) to avoid confusion

125

first list

125–126

whether the corresponding

126

second list

132–133

Ideally renames these as %m0, %m1, %cond0, %cond1, %r0, %r1

140–141

can we given them more descriptive names, e.g., %memref_base_pointers

345

not sure what that means

349

descriptive names

360

Add comment: // Zero initialize result buffer

365

also let's use descriptive names here

367

Let's add a comment: // Check for aliasing with retained memrefs

380

Also comment here

maerhart updated this revision to Diff 547149.Aug 4 2023, 2:55 AM
maerhart marked 20 inline comments as done.

Address comments

maerhart added inline comments.Aug 4 2023, 2:56 AM
mlir/lib/Conversion/BufferizationToMemRef/BufferizationToMemRef.cpp
345

I think this can go away now since we've added comments to the code below and more descriptive names.

springerm accepted this revision.Aug 4 2023, 5:02 AM
springerm added inline comments.
mlir/lib/Dialect/Bufferization/IR/BufferizationOps.cpp
776–777

When an op is changed in-place, it must be wrapped in rewriter.updateRootInPlace.

This revision is now accepted and ready to land.Aug 4 2023, 5:02 AM
maerhart updated this revision to Diff 547173.Aug 4 2023, 5:43 AM

add updateRootInPlace and clang-format

mlir/lib/Dialect/Bufferization/IR/BufferizationOps.cpp
776–777

Oh yes, we should not block application of further canonicalizations after this one only because we update in-place. Thanks for the catch, didn't pay enough attention.

This revision was automatically updated to reflect the committed changes.
maerhart marked an inline comment as done.