This is an archive of the discontinued LLVM Phabricator instance.

[mlir][CallOpInterface] Add `setCalleeFromCallable` method
ClosedPublic

Authored by Whitney on May 3 2023, 9:24 AM.

Details

Summary

Currently CallOpInterface has a method getCallableForCallee to have a consistent way to get the callee from an operation with CallOpInterface, but missing a consistent way to set a callee for an operation with CallOpInterface.

A set callee method is useful for transformations that operate on CallOpInterface, and change the callee, e.g., a pass that specialize function, which clone the callee, and change the CallOpInterface's callee to the cloned version. Without such method, transformation would need to understand the implementation for every operations with CallOpInterface, and have a type switch to handle them.

This review adds a method to set callee for operation with CallOpInterface.

Diff Detail

Event Timeline

Whitney created this revision.May 3 2023, 9:24 AM
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Whitney requested review of this revision.May 3 2023, 9:24 AM
gysit added a comment.May 4 2023, 12:16 AM

Can you explain the motivation for the change (ideally in the commit message)? Could you achieve your goal also by changing the specific call operation you are working with?

Also note that the unit tests are currently failing.

Whitney edited the summary of this revision. (Show Details)May 4 2023, 6:56 AM
Whitney edited the summary of this revision. (Show Details)May 4 2023, 7:03 AM
Whitney updated this revision to Diff 519478.May 4 2023, 7:09 AM

Can you explain the motivation for the change (ideally in the commit message)? Could you achieve your goal also by changing the specific call operation you are working with?

Added in commit message. I am working with a CallOpInterface, so it would need to have a big type switch to handle every possible call operation, which is not ideal.

Also note that the unit tests are currently failing.

Thanks for letting me know, updated the diff.

gysit added a comment.May 4 2023, 7:47 AM

Thanks for the longer commit message. It seems there is still a compilation error though.

Any chance you can add a test to exercise the new functionality upstream? It seems there is at least a test operation that implements the interface already but probably no pass or unit test where a test could be added easily.

zero9178 added inline comments.May 4 2023, 7:53 AM
mlir/include/mlir/Interfaces/CallInterfaces.td
47

Could you document here what the expectected preconditions and post conditions of the method are?

E.g. if I have a func.call op is it a precondition of the method that $callee must be a FlatSymbolRefAttr or is it a post condition that the implementation should then simply do nothing if its not. Vice versa with ops like func.call_indirect and Value.

I also think that describing the meaning of what the SSA value represents should not be described here, given the meaning is dependent upon the Op that creates the value and/or the call op that uses it (could e.g. also be a function pointer, doesn't have to be a lambda-like operation with a region).

Whitney updated this revision to Diff 519502.May 4 2023, 8:07 AM

Added mlir:: in FIROps.td.

Whitney updated this revision to Diff 519545.May 4 2023, 9:57 AM

changed comment.

Whitney marked an inline comment as done.May 4 2023, 10:04 AM

Thanks for the longer commit message. It seems there is still a compilation error though.

The compilation error should be fixed now.

Any chance you can add a test to exercise the new functionality upstream? It seems there is at least a test operation that implements the interface already but probably no pass or unit test where a test could be added easily.

There seems to have no extra testing for getCallableForCallee on top of the test operation. As you said, there seems to be no easy way to add extra testing. Do you have any suggestions?

mlir/include/mlir/Interfaces/CallInterfaces.td
47

Updated comment.

Whitney marked an inline comment as done.May 4 2023, 10:05 AM
gysit accepted this revision.May 4 2023, 11:11 PM

LGTM

There seems to have no extra testing for getCallableForCallee on top of the test operation. As you said, there seems to be no easy way to add extra testing. Do you have any suggestions?

I think ideally the pass exercising the new functionality would live upstream. But let's proceed with the revision as is then.

This revision is now accepted and ready to land.May 4 2023, 11:11 PM
This revision was landed with ongoing or failed builds.May 8 2023, 6:07 AM
This revision was automatically updated to reflect the committed changes.