Page MenuHomePhabricator

[mlir] Convert OpTrait::FunctionLike to FunctionOpInterface

Authored by rriddle on Jan 13 2022, 8:53 PM.



This commit refactors the FunctionLike trait into an interface (FunctionOpInterface).
FunctionLike as it is today is already a pseudo-interface, with many users checking the
presence of the trait and then manually into functionality implemented in the
function_like_impl namespace. By transitioning to an interface, these accesses are much
cleaner (ideally with no direct calls to the impl namespace outside of the implementation
of the derived function operations, e.g. for parsing/printing utilities).

I've tried to maintain as much compatability with the current state as possible, while
also trying to clean up as much of the cruft as possible. The general migration plan for
current users of FunctionLike is as follows:

  • function_like_impl -> function_interface_impl

Realistically most user calls should remove references to functions within this namespace
outside of a vary narrow set (e.g. parsing/printing utilities). Calls to the attribute name
accessors should be migrated to the FunctionOpInterface:: equivalent, most everything
else should be updated to be driven through an instance of the interface.

  • OpTrait::FunctionLike -> FunctionOpInterface

hasTrait checks will need to be moved to isa, along with the other various Trait vs
Interface API differences.

  • populateFunctionLikeTypeConversionPattern -> populateFunctionOpInterfaceTypeConversionPattern

Fixes #52917

Diff Detail

Event Timeline

rriddle created this revision.Jan 13 2022, 8:53 PM
rriddle requested review of this revision.Jan 13 2022, 8:53 PM



Indeed: since there is a $type, why isn't there a getType() already generated by ODS?

41 ↗(On Diff #399879)

I'm not fond of this: I would have a the concrete op be more explicit ($funcType or something like that) to avoid confusion with the "usual Type" we get on APIs which is about the Value domain.

On the other hand, we'll have the opportunity to have a FunctionType getType() method on the interface now and redirect users to it.

136 ↗(On Diff #399879)

Why isn't the default checking that getTypeAttr() is a FunctionType?

rriddle added inline comments.Jan 17 2022, 10:07 PM
136 ↗(On Diff #399879)

FunctionLike (now FunctionOpInterface) doesn't require a FunctionType. The type of the function is allowed to be represented by other types.

mehdi_amini added inline comments.Jan 17 2022, 10:52 PM
55–56 ↗(On Diff #399879)

I think the contract here is just that the operation has a getType() method that returns a type that has a getInputs() method :)

136 ↗(On Diff #399879)

Oh really? I see the doc on the other APIs actually, you could do the same here though: it seems that all the defaultImplementation are freely assuming it and come with:

Note: This method must be overridden if the function does not use
`mlir::FunctionType` as the underlying function type.
rriddle marked 5 inline comments as done.Jan 18 2022, 11:24 AM
rriddle added inline comments.

The builtin dialect is not yet generating the get* accessors.

41 ↗(On Diff #399879)

Even if we have a getType, we still end up setting the type and interacting with it via utility functions. I'm not claiming that the current setup is the best, but this patch in particular is trying to balance the fine line of NFC-ish refactoring from Trait->Interface, and cleaning up all of the lingering tech debt in FunctionLike. I've tried to keep things relatively close to what they were before, given that we already have quite a bit of code motion happening here.

55–56 ↗(On Diff #399879)

Indeed, restructured these to rely on the derived getType() having the desired methods (in the default implementation that is). As mentioned before, I've tried to keep things relatively close to what they are now. I'm hesitating to fix all of the current tech debt given that this commit is already quite a large refactoring (for some definition of large). Ideally in a followup, the dubious restrictions and way things are setup here can be restructured.

136 ↗(On Diff #399879)

This isn't assuming FunctionType, it's assuming that there are no additional invariants on the function type (however that type is represented). Just removed the default implementation though, better to make this explicit.

rriddle updated this revision to Diff 401026.Jan 18 2022, 3:59 PM
rriddle marked 4 inline comments as done.
mehdi_amini added inline comments.Jan 18 2022, 4:18 PM
41 ↗(On Diff #399879)

Can you include a TODO: this field will be renamed to something less generic?

Right we're documenting the interface expectations and changing this will be painful, but I think it is important to acknowledge that "type" as an attribute and the associated getters/setters isn't ideal for non-SSA value

mehdi_amini accepted this revision.Jan 18 2022, 4:18 PM

LG otherwise :)

This revision is now accepted and ready to land.Jan 18 2022, 4:18 PM
rriddle added inline comments.Jan 18 2022, 4:20 PM
41 ↗(On Diff #399879)

Yeah, right on. I'm all for adding in as many FIXME/TODOs as we can (there is a lot of mess here to clean up). Let me know if you see any other obvious spots to add a PLEASE_FIX_THIS (even if I submit before then, I'll add them as a followup to resolve).