Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[mlir] Gen removeAttr methods with tablegen

Authored by fdeazeve on Dec 29 2020, 8:44 AM.



If an operation defines an optional attribute (OptionalAttr or
UnitAttr), transformations may wish to remove these attributes while
maintaining invariants established by the operation. Currently, the only
way to do this is by calling Operation::removeAttr("attrName"), which
requires developers to know the exact name of the attribute used by
table-gen. Furthermore, if the attribute name changes, this won't be
detected at compile time. Instead, removeAttr would return an empty
attribute and no errors would be raised, unless the caller checks for
the returned value.

This patch adds table gen support for generating remove<AttrName>Attr
methods for OptionalAttributes defined by operations.

Implementation choice: to preserve camelCase for the method's name, the
first character of an attribute called myAttr is changed to upper case
in order to preserve the coding style, so the final method would be
called removeMyAttr.

Diff Detail

Event Timeline

fdeazeve created this revision.Dec 29 2020, 8:44 AM
fdeazeve requested review of this revision.Dec 29 2020, 8:44 AM
fdeazeve added a subscriber: bondhugula.EditedDec 29 2020, 8:47 AM

@bondhugula @mehdi_amini We briefly discussed this on discord a while ago. I'm not sure if this is the right implementation or if there is interest at all in this, please let me know your thoughts.

From a testing point of view, I couldn't find any unit tests for table gen generated methods. None of the upstream dialects seem to remove optional attributes from operations. Pointers would be appreciated.

mehdi_amini accepted this revision.Dec 29 2020, 9:01 AM
mehdi_amini added inline comments.

nit: in general we are conservative with auto and use it when it helps readability and/or the type is entirely obvious from the context.

This revision is now accepted and ready to land.Dec 29 2020, 9:01 AM
fdeazeve updated this revision to Diff 313976.Dec 29 2020, 9:35 AM

Update LIT and fix linter issue

fdeazeve added inline comments.Dec 29 2020, 9:38 AM

Yeah, I noticed this method was slightly violating the auto guidelines, but here I favoured the style already in use inside this file, i.e. inside the analogous method genAttrSetters (just above). I don't mind removing the autos if you believe this argument is not strong enough.

mehdi_amini added inline comments.Dec 29 2020, 10:41 AM

As long as you're aware of the guideline, it is fairly minor in this particular case.

@mehdi_amini Would you be able to help with merging this?

This revision was automatically updated to reflect the committed changes.
tpopp added a subscriber: tpopp.Jan 5 2021, 3:36 PM

I can handle this one, but just a note that this used Attribute instead of ::mlir::Attribute like all the other cases. This can fail then on any code not in an mlir namespace.

My bad! The mlir:: namespace was used just below some of the lines I touched, I should have seen it:(
I'll pay more attention to this in the future

tpopp added a comment.Jan 5 2021, 3:45 PM

No problem. is my fix CL. Please take a look if you have time. I just prepended ::mlir:: everywhere, but I didn't look deeply at the code (I'm running tests now, but they take a while for me).

Should we move the test dialect outside of the mlir namespace to catch this kind of things upstream?