This is an archive of the discontinued LLVM Phabricator instance.

[mlir][bufferization] Generalize and rename isMemoryWritten
ClosedPublic

Authored by springerm on Jan 13 2023, 5:45 AM.

Details

Summary

The name of the method was confusing. It is bufferizesToMemoryWrite, but from the perspective of OpResults.

bufferizesToMemoryWrite(OpResult) now supports ops with regions that do not have aliasing OpOperands (such as scf.if). These ops no longer need to implement isMemoryWrite.

Depends On: D141683

Diff Detail

Event Timeline

springerm created this revision.Jan 13 2023, 5:45 AM
Herald added a project: Restricted Project. · View Herald Transcript
springerm requested review of this revision.Jan 13 2023, 5:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2023, 5:45 AM
mlir/include/mlir/Dialect/Bufferization/IR/BufferizableOpInterface.h
379

Can we separate this change from this PR?
It seems it is unrelated.

541

Does this really need to be overridable?
If not consider taking out of the interface.

mlir/include/mlir/Dialect/Bufferization/IR/BufferizableOpInterface.td
106

Could we have some IR examples to support the cases ?

112

grammo

mlir/lib/Dialect/Bufferization/IR/BufferizableOpInterface.cpp
598

missing to

603

Do these correspond to the 3 cases in the doc?
If so, it would be good to give them a Case 1/2/3 to connect them.

603

This feels related to DpsInterface ?
Should be using that more in the impl of BufferizableOpInterface ?

651

can we drop the alwaysIncludeLeaves change in this PR and better justify it in a separate PR?

mlir/lib/Dialect/SCF/Transforms/BufferizableOpInterfaceImpl.cpp
128

Nice red here, above and below!

springerm updated this revision to Diff 490862.Jan 20 2023, 8:12 AM
springerm marked 9 inline comments as done.

address comments

mlir/include/mlir/Dialect/Bufferization/IR/BufferizableOpInterface.h
541

Yes, e.g., tensor.empty must override this. Added an example (Counter-Example) to the InterfaceMethod description.

mlir/lib/Dialect/Bufferization/IR/BufferizableOpInterface.cpp
603

We are using it already to provide a default implementation for DPS ops (DstBufferizableOpInterfaceExternalModel). We may be able to do more interesting things if op interfaces could inherit from another op interface.

LGTM for the very tiny sparse part ;-)

Fine to keep the inheritance part for a followup PR

mlir/lib/Dialect/Bufferization/IR/BufferizableOpInterface.cpp
603

We may be able to do more interesting things if op interfaces could inherit from another op interface.

See: https://reviews.llvm.org/D140198

This revision is now accepted and ready to land.Jan 30 2023, 12:02 AM