[PATCH] D74143: [MLIR] Adding attribute setters generation for table gen

[Lei Zhang via Phabricator via llvm-commits]

antiagainst accepted this revision.
antiagainst marked an inline comment as done.
antiagainst added a comment.
This revision is now accepted and ready to land.



> That seems independent of this though, or are you thinking the variants of attribute methods? E.g., we'll have multiple different convenience accessors generated for the same attribute beyond the raw and convenience one we have today?

Yup. That's my concern.

> Of course if it was named someStuff, then you'd have someStuff and someStuffAttr. But that does sort of bias it towards one naming convention.



> Without the prefix/postfix we don't need to mandate how ops are spelled and allow for dialect owners to specify the naming convention of their convenience accessors generated (e.g., some_stuff, someStuff, SomeStuff, Some_Stuff, Some_stufF all can be generated as convenience accessors today).

Actually I think the behavior of attaching `Attr` as a suffix to the raw attribute getter is forcing dialects to adopt camelCase for the attribute names; otherwise if one use snake_case, one just see weird names like some_stuffAttr. To truly allow dialect writers to dictate what naming style they want to use, the dialect writer need to control what naming convention is used to generate these attribute getter methods too.



================
Comment at: mlir/test/mlir-tblgen/op-attribute.td:64
+// DEF-NEXT:   this->setAttr("cAttr", attr);
+
 // Test build methods
----------------
jpienaar wrote:
> mehdi_amini wrote:
> > AlexEichenberger wrote:
> > > mehdi_amini wrote:
> > > > It is a bit annoying that a setters isn't prefixed with `set`: in general I rather have method starting with a verb expressing their actions. (This is also the general style in MLIR I believe).
> > > > 
> > > > That said we already emit the getters without a `get` similarly, so this is in line with what is already there.
> > > > 
> > > Would you like me to add the set, e.g. setPadAttr(...) or leave the proposal as is?
> > I'd leave it as-is for now, as it is aligned with the getter.
> > It is also a problem with the style (how do we make it CamelCase after the set?)
> > 
> > Seems like it goes beyond this revision, @antiagainst do you agree?
> I think leaving it as is and then revising post discussion. There is a lot of debate as to style. E.g., 
> - does a dialect have a style or is it uniform? (getX and setX is what you get, can't be set)
> - does that style impact the generated getters/setters? or does the style reflect the style variables are named as? (e.g, assertion of source state or target state)
> - is there reformatting of the variable's style or should it be simple and people defining it are responsible for correct naming? [e.g., a dialect sets prefix for get and set and it just gets prefixed vs variables are attempted to be reformatted to match a given style]
> - should it be customizable per op which convenience accessors are generated? :). 
> - should it be a function of the dialect or of the generation? (e.g., command line flag to mlir-tblgen)
> 
> Also I know of at least one group that want the equivalent for updating operands as you are doing here for attributes.
Yeah, I agree this is out of the scope of this patch. So I'm fine of letting this patch in. :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74143/new/

https://reviews.llvm.org/D74143