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

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar marked an inline comment as done.
jpienaar added a comment.

In D74143#1874608 <https://reviews.llvm.org/D74143#1874608>, @antiagainst wrote:

> I'm concerned about the method name here. Although we can rely on compiler symbol resolution, it's still quite confusing to see two attributes with the same name but one for getters and one for setters.


Maybe, https://en.wikipedia.org/wiki/Uniform_access_principle is another well established view here. I think it might depend on your first language, folks from Java or C++ first backgrounds may find get/set prefix second nature/almost required. But this is used for generating C++ code, so if it is confusing for folk then good to discuss.

> If we also think about the getter method returning "unwrapped" values, that's three.. So if I have a `some_stuff` attribute, we have `some_stuff()`, `some_stuffAttr()`, and `some_stuffAttr(some-params)`. It's also quite sad to see the mix of different naming conventions here.

I had initially proposed to have an attribute accesor generated so that would have `SomeOp::Attr(op).some_stuff()` or `op.attr().some_stuff()` instead of `op.some_stuffAttr()` with this being one of the reasons, but folks found the latter more natural. Also an option to revise that along with this 😉

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.

> And I think the number of attribute methods is just gonna to grow.

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?

> It would be far better if we have `getSomeStuff()`, `getSomeStuffAttr()`, and `setSomeStuff()`. FWIW, I still think having proper naming convention and method prefix/postfix control at dialect level is the better way to go given that we don't mandate how attributes should be named for ops.

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).



================
Comment at: mlir/test/mlir-tblgen/op-attribute.td:64
+// DEF-NEXT:   this->setAttr("cAttr", attr);
+
 // Test build methods
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74143