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

[Ding]

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.

[Ding]

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

[mehdi_amini]

Can you add tests demonstrating this in various scenarios?

[Ding]

In D136555#3878204, @mehdi_amini wrote:

Can you add tests demonstrating this in various scenarios?

OK， I‘m working on this.

[Ding]

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

[rriddle]

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]

In D136555#3916616, @rriddle wrote:

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)

[mehdi_amini]

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]

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

[Ding]

In D136555#3916616, @rriddle wrote:

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？

[rriddle]

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]

In D136555#3928502, @rriddle wrote:

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.

mlir/include/mlir/IR/OpBase.td

class Attr<Pred condition, string summary = ""> :
   // The value type of this attribute. This corresponds to the mlir::Type that
   // this attribute returns via `getType()`.
   Type valueType = ?;
 
   // Whether the attribute is optional. Typically requires a custom
   // convertFromStorage method to handle the case where the attribute is
   // not present.
   bit isOptional = 0;
   
+  // Whether the getter/setter function of the attribute will be hidden.
+  bit isHidden = 0;
+
   // What is the base-level Attr instantiation that this Attr is built upon.
   // Unset means this is a base-level Attr.
   //
   // This field is used by attribute wrapper classes (DefaultValuedAttr,
   // OptionalAttr, etc.) to retrieve the base-level attribute definition.
   // This can be used for getting its name; otherwise, we will see
   // "anonymous_<number>" as the attribute def name because of template
   // instantiation.
class OptionalAttr<Attr attr> : Attr<attr.predicate, attr.summary> {
   let convertFromStorage = "$_self ? " # returnType # "(" #
                            attr.convertFromStorage # ") : (::llvm::None)";
   let valueType = attr.valueType;
   let isOptional = 1;
 
   let baseAttr = attr;
 }
 
+// Decorates an attribute as hidden. The hidden attribute will not have their
+// getter/setter functions gererated.
+class HiddenAttr<Attr attr> : Attr<attr.predicate, attr.summary> {
+    // Rewrite the attribute to be hidden.
+    let storageType = attr.storageType;
+    let returnType = attr.returnType;
+    let convertFromStorage = attr.convertFromStorage;
+    let constBuilderCall = attr.constBuilderCall;
+    let isHidden = 1;
+    
+    let baseAttr = attr;
+}
+
 // Default-valued string-based attribute. Wraps the default value in escaped
 // quotes.
 class DefaultValuedStrAttr<Attr attr, string val>
     : DefaultValuedAttr<attr, "\"" # val # "\"">;
 class DefaultValuedOptionalStrAttr<Attr attr, string val>
     : DefaultValuedOptionalAttr<attr, "\"" # val # "\"">;
 
 //===----------------------------------------------------------------------===//

mlir/include/mlir/TableGen/Attribute.h

public:
   // Returns whether this attribute has a default value.
   bool hasDefaultValue() const;
   // Returns the default value for this attribute.
   StringRef getDefaultValue() const;
 
   // Returns whether this attribute is optional.
   bool isOptional() const;
 
+  // Returns whether this attribute is hidden.
+  bool isHidden() const;
+
   // Returns true if this attribute is a derived attribute (i.e., a subclass
   // of `DerivedAttr`).
   bool isDerivedAttr() const;
 
   // Returns true if this attribute is a type attribute (i.e., a subclass
   // of `TypeAttrBase`).
   bool isTypeAttr() const;
 

mlir/lib/TableGen/Attribute.cpp

 
 StringRef Attribute::getDefaultValue() const {
   const auto *init = def->getValueInit("defaultValue");
   return getValueAsString(init);
 }
 
 bool Attribute::isOptional() const { return def->getValueAsBit("isOptional"); }
 
+bool Attribute::isHidden() const { return def->getValueAsBit("isHidden"); }
+
 StringRef Attribute::getAttrDefName() const {
   if (def->isAnonymous()) {
     return getBaseAttr().def->getName();
   }
   return def->getName();
 }
 
 StringRef Attribute::getDerivedCodeBody() const {

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

+// RUN: mlir-tblgen -gen-op-decls -I %S/../../include %s | FileCheck %s
+
+// RUN: mlir-tblgen -gen-op-defs -I %S/../../include %s | FileCheck %s --check-prefix=DEFS
+
+include "mlir/IR/AttrTypeBase.td"
+include "mlir/IR/OpBase.td"
+
+def Test_Dialect : Dialect {
+  let name = "test";
+  let cppNamespace = "NS";
+}
+
+class NS_Op<string mnemonic, list<Trait> traits> :
+    Op<Test_Dialect, mnemonic, traits>;
+
+def AOp : NS_Op<"a_op", []> {
+  let arguments = (ins
+      I32Attr:$attrWithAccessor,
+      HiddenAttr<I32Attr>:$attrWithoutAccessor
+  );
+}
+
+// CHECK-LABEL: NS::AOp declarations
+// CHECK: getAttrWithAccessor
+// CHECK: setAttrWithAccessorAttr
+// CHECK: setAttrWithAccessor
+
+// CHECK-NOT: getAttrWithoutAccessor
+// CHECK-NOT: setAttrWithoutAccessorAttr
+// CHECK-NOT: setAttrWithoutAccessor
+
+// DEFS-LABEL: NS::AOp definitions
+
+// DEFS: AOpAdaptor::getAttrWithAccessorAttr
+// DEFS: AOpAdaptor::getAttrWithAccessor
+// DEFS: AOp::setAttrWithAccessorAttr
+// DEFS: AOp::setAttrWithAccessor
+
+// DEFS-NOT: AOpAdaptor::getAttrWithoutAccessorAttr
+// DEFS-NOT: AOpAdaptor::getAttrWithoutAccessor
+// DEFS-NOT: AOp::setAttrWithoutAccessorAttr
+// DEFS-NOT: AOp::setAttrWithoutAccessor
 No newline at end of file

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

method->body() << formatv(
         "  return {0}.{1}<{2}>();", emitHelper.getAttr(attrName),
         attr.isOptional() || attr.hasDefaultValue() ? "dyn_cast_or_null"
                                                     : "cast",
         attr.getStorageType());
   };
 
   for (const NamedAttribute &namedAttr : op.getAttributes()) {
     for (StringRef name : op.getGetterNames(namedAttr.name)) {
+      if (namedAttr.attr.isHidden()) {
+        continue;
+      }
       if (namedAttr.attr.isDerivedAttr()) {
         emitDerivedAttr(name, namedAttr.attr);
       } else {
         emitAttrWithStorageType(name, namedAttr.name, namedAttr.attr);
         emitAttrGetterWithReturnType(fctx, opClass, op, name, namedAttr.attr);
       }
     }
   }
if (attrValue)
       return (*this)->setAttr({0}AttrName(), {1});
     (*this)->removeAttr({0}AttrName());)";
     method->body() << formatv(
         optionalCodeBody, getterName,
         constBuildAttrFromParam(baseAttr, fctx, paramStr));
   };
 
   for (const NamedAttribute &namedAttr : op.getAttributes()) {
-    if (namedAttr.attr.isDerivedAttr())
+    if (namedAttr.attr.isDerivedAttr() || namedAttr.attr.isHidden())
       continue;
     for (auto [setterName, getterName] :
          llvm::zip(op.getSetterNames(namedAttr.name),
                    op.getGetterNames(namedAttr.name))) {
       emitAttrWithStorageType(setterName, getterName, namedAttr.attr);
       emitAttrWithReturnType(setterName, getterName, namedAttr.attr);
     }
   }
OpOperandAdaptorEmitter::OpOperandAdaptorEmitter(
   {
     auto *m = adaptor.addMethod("::mlir::DictionaryAttr", "getAttributes");
     ERROR_IF_PRUNED(m, "Adaptor::getAttributes", op);
     m->body() << "  return odsAttrs;";
   }
   for (auto &namedAttr : op.getAttributes()) {
     const auto &name = namedAttr.name;
     const auto &attr = namedAttr.attr;
-    if (attr.isDerivedAttr())
+    if (attr.isDerivedAttr() || attr.isHidden())
       continue;
     for (const auto &emitName : op.getGetterNames(name)) {
       emitAttrWithStorageType(name, emitName, attr);
       emitAttrGetterWithReturnType(fctx, adaptor, op, emitName, attr);
     }
   }
 
   unsigned numRegions = op.getNumRegions();