This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg][bufferize][NFC] Conflict detection funcs take OpOperand and OpResult
ClosedPublic

Authored by springerm on Oct 1 2021, 4:00 AM.

Details

Summary

By doing so, it is not necessary to get the OpOperand a second time via getAliasingOpOperand. Also, code slightly more readable because we do not have to deal with Optional<> return value.

Depends On D111380

Diff Detail

Event Timeline

springerm created this revision.Oct 1 2021, 4:00 AM
springerm requested review of this revision.Oct 1 2021, 4:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2021, 4:00 AM
mlir/lib/Dialect/Linalg/Transforms/ComprehensiveBufferize.cpp
2271

Please do not drop these comments as they are not trivial to infer.
A lot of load-bearing semantic is now carried by BufferRelation::None which hides this discussion.
We need to find a way to document this to avoid surprises in the future.

springerm updated this revision to Diff 377744.Oct 6 2021, 10:57 PM

Complete rewrite. Commit message to be updated.

springerm retitled this revision from [mlir][linalg][bufferize] Remove special case for ExtractSliceOp from analysis to [mlir][linalg][bufferize] Rewrite wouldCreateWriteToNonWriteableBuffer.Oct 6 2021, 11:02 PM
springerm edited the summary of this revision. (Show Details)
nicolasvasilache requested changes to this revision.Oct 6 2021, 11:45 PM
nicolasvasilache added inline comments.
mlir/lib/Dialect/Linalg/Transforms/ComprehensiveBufferize.cpp
2292

This is fishy: there may be ops in the future that have an input which aliases only a part of the result.
When bufferized, these don't have enough room to fit the result.

Putting a blocker until this is discussed.

This revision now requires changes to proceed.Oct 6 2021, 11:45 PM
springerm added inline comments.Oct 7 2021, 2:45 AM
mlir/lib/Dialect/Linalg/Transforms/ComprehensiveBufferize.cpp
2292

Currently, the entire machinery assumes (conservatively) that two tensors alias either entirely or not at all. It's not just getAliasingOpResult, but also wouldCreateReadAfterWriteInterference.

This is fishy: there may be ops in the future that have an input which aliases only a part of the result. When bufferized, these don't have enough room to fit the result.

I'm trying to understand when there would not be enough room to fit the result. E.g., this would be an op where the result tensor is larger than the argument tensor? This would require some redesign. There has to be an allocation somewhere. We currently avoid these cases by having explicit "output" tensors (like in linalg.generic). Then we would always have a suitable tensor (with the right size) to alias with.

If we have to support such ops that we cannot change, we have to find a different solution.

However, my thinking is: If getAliasingOpResult returns an OpResult and getInplaceableOpResult returns null (that's presumably the case you are concerned about; and that's also the case for ExtractSliceOp), the OpOperand is guaranteed to *not* bufferize to a memory write.

"Proof" by contradiction: Let's assume that:

  • getAliasingOpResult returns an OpResult
  • getInplaceableOpResult returns null
  • The OpOperand bufferizes to a memory write

Since getInplaceableOpResult is null and the operation is writing, it must create a new allocation and copy, i.e., bufferize out-of-place. But in that case, it cannot have an aliasing OpResult. Does this make sense?

springerm updated this revision to Diff 377819.Oct 7 2021, 6:13 AM

address comments

springerm marked an inline comment as done.Oct 7 2021, 6:14 AM
springerm added inline comments.
mlir/lib/Dialect/Linalg/Transforms/ComprehensiveBufferize.cpp
2292

As discussed, special handling of ExtractSliceOp for the moment.

springerm edited the summary of this revision. (Show Details)Oct 7 2021, 6:15 AM
springerm added inline comments.
mlir/lib/Dialect/Linalg/Transforms/ComprehensiveBufferize.cpp
2271

Moved into the body of mlir::linalg::inPlaceAnalysis.

nicolasvasilache requested changes to this revision.Oct 7 2021, 10:39 AM

seeing some feature creep here, please split and land the trivial pieces out first and then let's iterate on a more robust API / preconditions.

mlir/lib/Dialect/Linalg/Transforms/ComprehensiveBufferize.cpp
564

This fix is independent, please split it in a separate revision.

692

this NFC API change can be in its own CL, please do that and land it without review to reduce the amount of changes in this revision.

741

this NFC API change can be in its own CL, please do that and land it without review to reduce the amount of changes in this revision.

880–881

I don't understand why this API needs to change in this revision.
Losing the connection between opOperand and opResult is problematic to me here and below.

At the very least we should have strong documentation and loud assertion about the cases we allow.

974

losing the connection between opOperand and opResult is problematic to me here.

979

Since we lost the connection between opOperand and opResult, opOperand may be readonly and it does not make sense to call aliasesNonWritableBuffer on it.

2277

Let's call this one bufferizableInPlaceAnalysisImpl and add some assert that operand and result are related and cannot be random things. Atm we lose this information and it is unclear who should call what.

Let's keep the ExtractSliceOp version and make it call the Impl.

2332

Let's not jump the step here, call a bufferizableInPlaceAnalysis(ExtractSliceOp which can in turn call the lower level bufferizableInPlaceAnalysisImpl.

This revision now requires changes to proceed.Oct 7 2021, 10:39 AM
springerm retitled this revision from [mlir][linalg][bufferize] Rewrite wouldCreateWriteToNonWriteableBuffer to [mlir][linalg][bufferize][NFC] Conflict detection funcs take OpOperand and OpResult.Oct 7 2021, 8:19 PM
springerm edited the summary of this revision. (Show Details)
springerm edited the summary of this revision. (Show Details)Oct 7 2021, 8:22 PM
springerm marked 7 inline comments as done.Oct 7 2021, 8:34 PM
springerm added inline comments.
mlir/lib/Dialect/Linalg/Transforms/ComprehensiveBufferize.cpp
880–881

This was just for convenience. Since we already have the OpOperand in the caller. We could add another assert here, expand the comment to say that they have to match, or just abandon this revision. Either one is fine with me.

I just think that the Optional<OpOperand *> leads to unnecessarily complex (hard to parse understand) code.

979

aliasesNonWritableBuffer can be called on any Value. Should not be a problem.

The main change around this location has moved to D111379. While the new version may cover a few cases that may never appear in reality, I find it easier to understand.

In particular, I was thinking for a while that the existing conflict detection code non-writable buffer is missing a case.

%r = tensor.extract_slice %t

* %t aliases in-place write
* %r aliases non-writable buffer

This case is not covered by the existing code. The new one (D111379) does. I now believe that it is not possible to construct such an example. However, this was not immediately obvious to me. So for the sake of simplicity/understandability, my suggestion would be to go with a version similar to D111379.

springerm marked an inline comment as done.Oct 10 2021, 5:54 PM

Added asserts to functions.

springerm updated this revision to Diff 378543.Oct 10 2021, 6:49 PM

address comments

nicolasvasilache accepted this revision.Oct 12 2021, 7:41 AM
nicolasvasilache added inline comments.
mlir/lib/Dialect/Linalg/Transforms/ComprehensiveBufferize.cpp
880–881

with the assert, this WFM, thanks!

This revision is now accepted and ready to land.Oct 12 2021, 7:41 AM
This revision was landed with ongoing or failed builds.Oct 12 2021, 5:22 PM
This revision was automatically updated to reflect the committed changes.