Page MenuHomePhabricator

Add support to disable generation of setter/getter of attrs in ODS
Needs ReviewPublic

Authored by Ding on Oct 23 2022, 9:47 AM.

Details

Summary

Add a flag named isHidden in Attribute class. Op attribute defined
in td files with isHidden flag will not have their getter/setter
function generated in the Op class definition file.

Diff Detail

Event Timeline

Ding created this revision.Oct 23 2022, 9:47 AM
Ding updated this revision to Diff 470026.Oct 23 2022, 6:00 PM

Updating D136555: Add support to disable generation of setter/getter of attrs in ODS

Can you add tests demonstrating this in various scenarios?

Ding added a comment.Oct 23 2022, 10:49 PM

Can you add tests demonstrating this in various scenarios?

OK, I‘m working on this.

Ding updated this revision to Diff 473681.Nov 7 2022, 8:01 AM

Updating D136555: Add support to disable generation of setter/getter of attrs in ODS

Ding published this revision for review.Nov 8 2022, 10:20 PM

This doesn't really feel like an abstraction that should be on the Attribute class, this feels closer to something that should be on the Arg class. IMO this is not about controlling the definition of an attribute, but of tweaking how the op generation happens.

Ding added a comment.Nov 12 2022, 2:22 AM

This doesn't really feel like an abstraction that should be on the Attribute class, this feels closer to something that should be on the Arg class. IMO this is not about controlling the definition of an attribute, but of tweaking how the op generation happens.

I think the isHidden flag is just like the isOptional to decripe the generation dehavior of a single argument, and this flag has to be on a particular argument (since we might want to hide the getter/setter functions of some arguments, but not all of them)

LGTM, but I'd like River's approval (I'm not sure I understood well what he would like to see changed here)

mlir/test/mlir-tblgen/op-hidden-attribute.mlir
20

Typo? attrWithoutAccesser -> attrWithoutAccessor?

Ding updated this revision to Diff 474983.Nov 13 2022, 12:03 AM

Updating D136555: Add support to disable generation of setter/getter of attrs in ODS

Ding added a comment.Nov 13 2022, 12:27 AM

This doesn't really feel like an abstraction that should be on the Attribute class, this feels closer to something that should be on the Arg class. IMO this is not about controlling the definition of an attribute, but of tweaking how the op generation happens.

Hi River, actually I am not sure I understood well what you would like to see. Could you please explain it more specifically?

Sorry for the delay, lots of stuff internally.

The use of optional doesn't feel the same as what's described here, but I suppose it does somewhat match what we've already done with the "derived" attribute stuff. It'd be nice to clean up the attribute/argument/etc abstractions in ODS at some point, but I won't block on that.

Can we keep the Attr accessors? I can understand getting rid of the getFoo/setFoo accessors (if you want special behavior there), but I'm somewhat concerned that we are going to be creating a state of inconsistent API availability, and we often rely on certain accessors being available. The Attr accessors also handle the "good" practice of setting the attributes in the optimal/expected way.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1018–1020

style; Can you drop the trivial braces here?

Ding added a comment.Nov 20 2022, 7:47 PM

Sorry for the delay, lots of stuff internally.

The use of optional doesn't feel the same as what's described here, but I suppose it does somewhat match what we've already done with the "derived" attribute stuff. It'd be nice to clean up the attribute/argument/etc abstractions in ODS at some point, but I won't block on that.

Can we keep the Attr accessors? I can understand getting rid of the getFoo/setFoo accessors (if you want special behavior there), but I'm somewhat concerned that we are going to be creating a state of inconsistent API availability, and we often rely on certain accessors being available. The Attr accessors also handle the "good" practice of setting the attributes in the optimal/expected way.

Thanks for your reply. The reason I want to hide the accessors of certain Attr is that since we have implemeted another accessor and do some special behavior there, we do not want the developer to use the original accessors any more. We can keep the Attr accessors and use some developer guide thing to avoid developer's misusing, but I think it might be better to avoid this problem from the source.