Page MenuHomePhabricator

[mlir] Added new AllocationOpInterface and adapted BufferDeallocation pass to use the new interface.
ClosedPublic

Authored by dfki-mako on Sep 7 2021, 3:24 AM.

Details

Summary

The motivation behind this CL is the fact that the current BufferDeallocation pass supports building of tightly coupled DellocOp/CloneOp operations only. There have been many requests from the community to relax these restrictions in order to apply the BufferDeallocation pass to more domains and dialects. For this reason, this CL includes a newly introduced AllocationOpInterface to give developers the opportunity to implement their own "compatible" clone and deallocation operations.

  • Introduced AllocationOpInterface to create deallocation operations on-the-fly that are compatible with the allocation operation implementing this interface.
    • Added interface implementations for AllocOp and CloneOp defined in the MemRef dialect.
    • Adapted the BufferDeallocation pass to be compatible with the interface introduced in this CL.

Diff Detail

Event Timeline

dfki-mako created this revision.Sep 7 2021, 3:24 AM
dfki-mako requested review of this revision.Sep 7 2021, 3:24 AM
dfki-mako retitled this revision from Introduced AllocationOpInterface to create deallocation operations on-the-fly that are compatible with the allocation operation implementing this interface. Added interface implementations for AllocOp and CloneOp defined in the MemRef diallect. to [mlir] Added new AllocationOpInterface and adapted BufferDeallocation pass to use the new interface..Sep 7 2021, 3:26 AM
dfki-mako edited the summary of this revision. (Show Details)
dfki-mako edited the summary of this revision. (Show Details)
rriddle requested changes to this revision.Sep 7 2021, 3:41 AM

Can you also include the "why" in your commit description? The motivation isn't clear from the patch description.

mlir/include/mlir/Interfaces/SideEffectInterfaces.td
23–40

Why an OpInterface instead of a dialect interface? This kind of feels off for an op level granularity, and seems to be more related to transformation.

31

Did you intend for this method to be static?

36–37

Is this failable?

mlir/lib/Transforms/BufferDeallocation.cpp
108–125
549–559
This revision now requires changes to proceed.Sep 7 2021, 3:41 AM

Thanks for the patch!

herhut added inline comments.Sep 7 2021, 7:46 AM
mlir/include/mlir/Interfaces/SideEffectInterfaces.td
36

Looking at this change, I realized we likely also need a buildClone method here.

mlir/lib/Transforms/BufferDeallocation.cpp
108–125

With this only the first operation that is lacking the interface is reported. Is that generally preferred in MLIR, abort on first failure? Otherwise I find the current version more helpful.

119

As this iterates over all operations, maybe phrase this in terms of the operation? Something like `allocated value is not deallocated nor does the operation implement the AllocOpInterface"?

550

I think it would be better to find the alloc that was cloned here. Do we have the corresponding aliasing information available here?

While currently this can only be a clone, at some point we will need to support different clone operations for the different allocs, for the same reason that this supports different free operations.

dfki-mako updated this revision to Diff 372454.Sep 14 2021, 3:56 AM
dfki-mako marked 6 inline comments as done.

Added new buildClone operation to the newly introduced AllocationOpInterface.
Made Op interface methods static.
Added support for automatic building of "compatible" deallocation/clone operations based on inverse alias information.
Added support for building "default" DeallocOp/CloneOp values in the case of unknown value sources (e.g. function parameters).
Refined verification part to work on logical result values.

dfki-mako edited the summary of this revision. (Show Details)Sep 14 2021, 4:00 AM
dfki-mako marked 3 inline comments as done.Sep 14 2021, 4:06 AM
dfki-mako added inline comments.
mlir/include/mlir/Interfaces/SideEffectInterfaces.td
23–40

We have also been thinking about this question before. Implementing a dialect interface seems to be more suitable on first sight. However, similar (if not even the same...) questions arise in this scope: should deallocation/clone operations be created on the type level rather than the Op/Dialect level? Should we support cases in which the operations can decide on different dealloc/clone ops depending on the context?

As you have also mentioned, an OP interface feels more related to transformations in general. That's why we have decided to go for the Op interface first. However, this is TBD :-)

36–37

An excellent questions. Currently, we assume that this operation cannot "fail" in any case, as we are implementing this interface for allocation operations that support these functions only. However, you can argue that these methods should have the ability to fail in general. In this case, we only need to adapt the BufferDeallocation pass to report proper errors :-) What do you think?

mlir/lib/Transforms/BufferDeallocation.cpp
108–125

We assumed that reporting all errors (instead of the first one) should be the preferred way of error reporting. However, we addressed @rriddle's comment by reporting the first error only for now.

herhut accepted this revision.Sep 14 2021, 4:35 AM

Looks good to me. Please also wait for @rriddle.

@aartbik would this allow using the deallocation pass with sparse or would more be needed?

mlir/include/mlir/Interfaces/SideEffectInterfaces.td
34

nit: missing ). Also, do you mean that the value is a result of the current Op?

42

nit: missing )

mlir/lib/Transforms/BufferDeallocation.cpp
211

You could use AllocationOpInterface definingOp = alloc.getDefiningOp<AllocationOpInterface>() here to simplify the code below.

603

Is this abstraction worth the reuse you get?

rriddle requested changes to this revision.Sep 20 2021, 10:17 AM
rriddle added inline comments.
mlir/include/mlir/Interfaces/SideEffectInterfaces.td
36–37

If the method can't fail, it feels a tad weird for an interface that is named as generally as AllocationOpInterface. I wouldn't expect an alloca operation to have a dealloc operation, but I would still consider it an "AllocationOp". Is there a better name for this interface? Or, how do you see the functionality of this interface growing/changing? Right now it seems very limited in focus. If we see this interface growing as a general AllocOp interface, I would make this method fail-able.

mlir/lib/Transforms/BufferDeallocation.cpp
214
625–629
640

Prefer using the global failed/success functions.

644–648
This revision now requires changes to proceed.Sep 20 2021, 10:17 AM

Sorry for the wait, coming back from OOO.

dfki-mako updated this revision to Diff 373829.Sep 21 2021, 3:06 AM
dfki-mako marked 12 inline comments as done.

Addressed reviewer comments and added support for failing interface functions.

dfki-mako added inline comments.Sep 21 2021, 3:08 AM
mlir/include/mlir/Interfaces/SideEffectInterfaces.td
36–37

After rethinking the design of our interface proposal, we also agree that these methods should be fail-able. Therefore, we changed their returns types to Optional<T> values.

rriddle added inline comments.Sep 21 2021, 2:01 PM
mlir/include/mlir/Interfaces/SideEffectInterfaces.td
25–26

Can you reword this to better capture what this interface is intended to represent in general? If the deallocation operations are optional, I imagine we want a more general description here.

36
39

Do you want to add a default implementation to these methods that returns None?

46
mlir/lib/Transforms/BufferDeallocation.cpp
215

?

612

This will result in a crash/segfault. Please use an appropriate error handling here, e.g. assert, llvm::report_fatal_error, propagate the error up, etc.

615–616
631–633

Same as above, please add proper error handling after the error.

636
dfki-mako updated this revision to Diff 374175.Sep 22 2021, 3:57 AM
dfki-mako marked 9 inline comments as done.

Addressed reviewer comments and enhanced error handling.

mlir/lib/Transforms/BufferDeallocation.cpp
215

Hmm... I am not sure, as alloc is a Value and not an Operation*..?

612

We enhanced our error handling logic to avoid crashes. However, the currently implemented method simply skips certain parts of the program without actually signaling a pass failure on the function level. Do you suggest that the pass should also signal a pass failure in these cases?

rriddle added inline comments.Sep 22 2021, 12:40 PM
mlir/lib/Transforms/BufferDeallocation.cpp
215

(Thanks, I'm blind)

612

It's up to you how to handle the error. If you do a runtime recoverable error, you'll need to propagate failure all the way up to the top of the pass and signalPassFailure(in general I wouldn't expect the compiler to keep running after an error). If you don't expect this to be recoverable at runtime (i.e. you don't expect this type of input), I'd just assert/report_fatal_error/etc right after the error.

631–633

Is the cast to logical result really necessary here?

dfki-mako updated this revision to Diff 375539.Sep 28 2021, 5:30 AM
dfki-mako marked 2 inline comments as done.

Updated error handling to propagate errors upwards in order to signal pass failures.

mlir/lib/Transforms/BufferDeallocation.cpp
612

All right. Consequently, we adjusted the error propagation to abort on the first failure during the transformation by signaling a pass failure.

631–633

The reason for this cast operation is the fact the we need to explicitly convert an mlir::InFlightDiagnostics result to an mlir::FailureOr<Value> instance.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 29 2021, 6:55 AM
This revision was automatically updated to reflect the committed changes.