[MLIR] Adding attribute setters generation for table gen

[AlexEichenberger]

Summary

In some dialects, attributes may have default values that may be determined only after shape inference. For example, attributes that are dependent on the rank of the input cannot be assigned a default value until the rank of the tensor is inferred.

While we can set attributes without explicit setters, referring to the attributes via accessors instead of having to use the string interface is better for compile time verification.

The proposed patch add one method per operation attribute that let us set its value. The code is a very small modification of the existing getter methods.

[rriddle]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

393

What is the context used for?

404

nit: Remove the extra space, and drop the trivial braces.

[AlexEichenberger]

updated style

[AlexEichenberger]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

393

It is used in the same context as the getters. In our case, we use this functionality in a shape inference pass. Is it at risk to vanish before a subsequent pass?

404

done

[mehdi_amini]

Can this be tested in-tree?

[jpienaar]

In D74143#1865419, @mehdi_amini wrote:

Can this be tested in-tree?

+1, thanks

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

388–390

Reflow?

// Generate raw named setter type. This is a wrapper class that allows setting
// the attributes via setters instead of having to use the string interface
// for better compile time verification.

This comment may also be more relevant inside: e.g., there are potentially multiple setters, as with getters, and this is one specific to raw setters.

393

To rephrase the question: fctx does not seemed to be used anywhere here and so could be removed.

[AlexEichenberger]

added tests of setters, removed unneeded code, edited & migrated comments as requested

[AlexEichenberger]

tx for the valuable input

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

388–390

performed reflow and moved comment to requested spot

393

good catch, sorry I missed the meaning of context here

[antiagainst]

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. 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. And I think the number of attribute methods is just gonna to grow. 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.

[mehdi_amini]

mlir/test/mlir-tblgen/op-attribute.td

64 ↗

(On Diff #243928)

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.

[AlexEichenberger]

mlir/test/mlir-tblgen/op-attribute.td

64 ↗

(On Diff #243928)

Would you like me to add the set, e.g. setPadAttr(...) or leave the proposal as is?

[mehdi_amini]

LGTM, if @antiagainst approves.

mlir/test/mlir-tblgen/op-attribute.td

64 ↗

(On Diff #243928)

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?

[jpienaar]

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

mlir/test/mlir-tblgen/op-attribute.td

64 ↗

(On Diff #243928)

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.

[AlexEichenberger]

@ antiagainst

What do you suggest I do? Would you like me to introduce the get and set, and leave the older methods for deprecation? Or only add the setter with "set" in the name?

In 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. 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. And I think the number of attribute methods is just gonna to grow. 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.

[antiagainst]

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.

mlir/test/mlir-tblgen/op-attribute.td

64 ↗

(On Diff #243928)

Yeah, I agree this is out of the scope of this patch. So I'm fine of letting this patch in. :)

[mehdi_amini]

In D74143#1881674, @antiagainst wrote:

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.

I suspect any attempt to provide such facility (switching naming convention) will hit the issue of mismatch with Traits and inherited properties: I think it will require a major change in our infrastructure to provide this.

[antiagainst]

In D74143#1884098, @mehdi_amini wrote:

In D74143#1881674, @antiagainst wrote:

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.

I suspect any attempt to provide such facility (switching naming convention) will hit the issue of mismatch with Traits and inherited properties: I think it will require a major change in our infrastructure to provide this.

I don't quite see the reason. FWIW, the current way is also a naming convension; it can also hit the issue of mismatch with traits and inherited properties. And once that happens, the writer has no way to avoid but to change the attribute name at the moment. Providing the ability to choose actually mitigates that...

[mehdi_amini]

In D74143#1889818, @antiagainst wrote:

In D74143#1884098, @mehdi_amini wrote:

In D74143#1881674, @antiagainst wrote:

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.

I suspect any attempt to provide such facility (switching naming convention) will hit the issue of mismatch with Traits and inherited properties: I think it will require a major change in our infrastructure to provide this.

I don't quite see the reason. FWIW, the current way is also a naming convension; it can also hit the issue of mismatch with traits and inherited properties. And once that happens, the writer has no way to avoid but to change the attribute name at the moment. Providing the ability to choose actually mitigates that...

I see two entirely separated problems:

1. Naming convention with respect to collision management: this is a correctness question. The C++ language does not allow easily to "namespace" methods, we could (and should) look into ways make our scheme more robust while keeping our modularity.
2. Naming convention with respect to style guide: this is a stylistic question (CamelCase vs camelCase vs ...). This is what I was referring to: offering an Op interface that has a consistent tweak-able style does not seem easy (if doable at all to me).

[mehdi_amini]

I wonder if emitting the accessors in a separate class and inheriting from it wouldn't be already a nicer intermediate mode: we'd hit a compile time failure only if/when the overloaded conflicting method is accessed, and it can be addressed at call site by making the access to the desired base class explicit.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

private:
   void genOpAsmInterface();
 
   // Generates the `getOperationName` method for this op.
   void genOpNameGetter();
 
   // Generates getters for the attributes.
   void genAttrGetters();
 
+  // Generates setter for the attributes.
+  void genAttrSetters();
+
   // Generates getters for named operands.
   void genNamedOperandGetters();
 
   // Generates getters for named results.
   void genNamedResultGetters();
 
   // Generates getters for named regions.
   void genNamedRegionGetters();
OpEmitter::OpEmitter(const Operator &op)
   // Generate C++ code for various op methods. The order here determines the
   // methods in the generated file.
   genOpAsmInterface();
   genOpNameGetter();
   genNamedOperandGetters();
   genNamedResultGetters();
   genNamedRegionGetters();
   genAttrGetters();
+  genAttrSetters();
   genBuilder();
   genParser();
   genPrinter();
   genVerifier();
   genCanonicalizerDecls();
   genFolderDecls();
   genOpInterfaceMethods();
   generateOpFormat(op, opClass);
if (attr.isDerivedAttr()) {
       emitDerivedAttr(name, attr);
     } else {
       emitAttrWithStorageType(name, attr);
       emitAttrWithReturnType(name, attr);
     }
   }
 }
 
+// Generate raw named setter type. This is a wrapper class that allows
+// setting to the attributes via setters instead of having to use
+// the string interface for better compile time verification.
+void OpEmitter::genAttrSetters() {
+  FmtContext fctx;
+  fctx.withBuilder("mlir::Builder(this->getContext())");
+  auto emitAttrWithStorageType = [&](StringRef name, Attribute attr) {
+    auto &method = opClass.newMethod("void", (name + "Attr").str(), 
+        (attr.getStorageType() + " attr").str());
+    auto &body = method.body();
+    body << "  this->setAttr(\"" << name << "\", attr);";
+  };
+
+  for (auto &namedAttr : op.getAttributes()) {
+    const auto &name = namedAttr.name;
+    const auto &attr = namedAttr.attr;
+    if (! attr.isDerivedAttr()) {
+      emitAttrWithStorageType(name, attr);
+    }
+  }
+}
+
 // Generates the named operand getter methods for the given Operator `op` and
 // puts them in `opClass`.  Uses `rangeType` as the return type of getters that
 // return a range of operands (individual operands are `Value ` and each
 // element in the range must also be `Value `); use `rangeBeginCall` to get
 // an iterator to the beginning of the operand range; use `rangeSizeCall` to
 // obtain the number of operands. `getOperandCallPattern` contains the code
 // necessary to obtain a single operand whose position will be substituted
 // instead of