Page MenuHomePhabricator

[mlir] Add DecomposeCallGraphTypes pass.
ClosedPublic

Authored by silvas on Nov 5 2020, 8:39 PM.

Details

Summary

This replaces the old type decomposition logic that was previously mixed
into bufferization, and makes it easily accessible.

This also deletes TestFinalizingBufferize, because after we remove the type
decomposition, it doesn't do anything that is not already provided by
func-bufferize.

Diff Detail

Event Timeline

silvas created this revision.Nov 5 2020, 8:39 PM
silvas requested review of this revision.Nov 5 2020, 8:39 PM
rriddle added inline comments.Nov 5 2020, 8:43 PM
mlir/lib/Transforms/DecomposeCallGraphTypes.cpp
1 ↗(On Diff #303322)

(Side nit, but this pass isn't general so I'd prefer to put it in Standard/Transforms instead of here.)

silvas updated this revision to Diff 303549.Nov 6 2020, 2:34 PM

Move to Standard/Transforms

bondhugula requested changes to this revision.Nov 9 2020, 10:29 AM

This appears nice to me. Some superficial comments to start with.

mlir/include/mlir/Dialect/StandardOps/Transforms/DecomposeCallGraphTypes.h
26–30

This description is a bit cryptic - could you please rephrase if possible?

mlir/lib/Dialect/StandardOps/Transforms/DecomposeCallGraphTypes.cpp
97

Doc comment here please.

119

Likewise.

142

Nit: int -> unsigned?

mlir/test/Transforms/decompose-call-graph-types.mlir
18

Do you think it's useful to also test cases where the arguments are a mix of tuples and non-tuple types? (and similarly return values).

mlir/test/lib/Transforms/TestDecomposeCallGraphTypes.cpp
20

Is this a TODO?

This revision now requires changes to proceed.Nov 9 2020, 10:29 AM
silvas updated this revision to Diff 303961.Nov 9 2020, 12:36 PM
silvas marked 2 inline comments as done.

Address feedback.

mlir/include/mlir/Dialect/StandardOps/Transforms/DecomposeCallGraphTypes.h
26–30

Tried to limit the jargon to a note.

mlir/lib/Dialect/StandardOps/Transforms/DecomposeCallGraphTypes.cpp
142

Switched to use unsigned consistently in the file.

mlir/test/Transforms/decompose-call-graph-types.mlir
18

Good catch! Done!

mlir/test/lib/Transforms/TestDecomposeCallGraphTypes.cpp
20

Sorry, remnant of something that I should have done before uploading. (for me, "XXX" = "TODO before uploading")

Fixed.

Is "call graph types" the best term here? This doesn't have to do with call graphs but with function signatures.(?) The older naming along the lines of FunctionSignatureConversion or DecomposeFuncSignature or UnpackFuncSignature looks clearer than anything with "CallGraph".

@dfki-mako or other contributors to previous buffer conversion are probably the best to review details here.

dfki-mako accepted this revision.Nov 12 2020, 1:28 AM

Awesome work 🚀 I don't see any problems to land this CL after addressing the nits 🤓

mlir/lib/Dialect/StandardOps/Transforms/DecomposeCallGraphTypes.cpp
109

nit: auto -> Value

136

nit: auto -> Value

147

nit: auto -> OpResult

dfki-heme accepted this revision.Nov 12 2020, 1:32 AM

Is "call graph types" the best term here? This doesn't have to do with call graphs but with function signatures.(?) The older naming along the lines of FunctionSignatureConversion or DecomposeFuncSignature or UnpackFuncSignature looks clearer than anything with "CallGraph".

I'm using the term "call graph types" as a stand-in for "function signatures, call signatures, and return signatures". That is, types that flow along call graph edges.

I was also keeping an eye towards eventually generalizing this beyond std.func/call/return. In that more general setting, I think "call graph" is much clearer, because different dialects might have objects in their call graphs that are not called "function". For example, the concept of a first class callable object has many different names: lambdas, closures, delegates. Using the name "function" doesn't sit well with me in that more general setting. In all cases though, the set of patterns we need are the same: one on CallOpInterface, one on CallableOpInterface, and one on something that represents returning control flow from a CallableOpInterface. Once we have extended those interfaces appropriately, the patterns in this pass would be updated to MatchAnyOpTypeTag patterns written over those interfaces and everything would just work.

I guess it depends on how one looks at the pass: is the pass fundamentally updating function signatures / callable signatures (and everything else is just changing to match), or is the pass to be viewed as decomposing types along call graph edges. I was thinking about the pass as the latter. What do you think?

silvas updated this revision to Diff 304909.Nov 12 2020, 11:10 AM
silvas marked 2 inline comments as done.

Address comments.

silvas marked an inline comment as done.Nov 12 2020, 11:10 AM
bondhugula accepted this revision.Nov 16 2020, 9:40 AM

LGTM - thanks! Just some minor comments (please see if the "mixed" test case could be improved for coverage).

mlir/lib/Dialect/StandardOps/Transforms/DecomposeCallGraphTypes.cpp
34–35

Triple /// comment.

78

A comment here perhaps.

mlir/test/Transforms/decompose-call-graph-types.mlir
103

Would it be useful to have a test case where you have a non-trivial tuple (at least one item in the tuple) mixed with a non-tuple?

mlir/test/lib/Transforms/TestDecomposeCallGraphTypes.cpp
32

auto -> ModuleOp

This revision is now accepted and ready to land.Nov 16 2020, 9:40 AM
This revision was automatically updated to reflect the committed changes.
silvas marked 2 inline comments as done.
silvas marked an inline comment as done.Nov 16 2020, 12:38 PM
silvas added inline comments.
mlir/test/Transforms/decompose-call-graph-types.mlir
103

Good catch! I updated it to exercise the 1:0 1:1 and 1:N cases.

Is "call graph types" the best term here? This doesn't have to do with call graphs but with function signatures.(?) The older naming along the lines of FunctionSignatureConversion or DecomposeFuncSignature or UnpackFuncSignature looks clearer than anything with "CallGraph".

I'm using the term "call graph types" as a stand-in for "function signatures, call signatures, and return signatures". That is, types that flow along call graph edges.

I was also keeping an eye towards eventually generalizing this beyond std.func/call/return. In that more general setting, I think "call graph" is much clearer, because different dialects might have objects in their call graphs that are not called "function". For example, the concept of a first class callable object has many different names: lambdas, closures, delegates. Using the name "function" doesn't sit well with me in that more general setting. In all cases though, the set of patterns we need are the same: one on CallOpInterface, one on CallableOpInterface, and one on something that represents returning control flow from a CallableOpInterface. Once we have extended those interfaces appropriately, the patterns in this pass would be updated to MatchAnyOpTypeTag patterns written over those interfaces and everything would just work.

I guess it depends on how one looks at the pass: is the pass fundamentally updating function signatures / callable signatures (and everything else is just changing to match), or is the pass to be viewed as decomposing types along call graph edges. I was thinking about the pass as the latter. What do you think?

  1. I guess "function signature" is the key thing - callable signatures and return signatures can be viewed as dependent/anciliary.
  2. Reg. the naming reflecting a future generalization: this is a frequent slippery slope. I've seen it happen multiple times when the thing is named based on what the plan is - making it harder to find and not reflect the current functionality. The other things you refer to for the future are probably also FunctionLike or at least callable? The 'call graph types' doesn't really bring out "types along call graph edges". DecomposeCallableSignature perhaps?

Anyway, all of this isn't really a major concern.