[mlir] Gen removeAttr methods with tablegen

[fdeazeve]

Summary

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.

[fdeazeve]

@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]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

805

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.

[fdeazeve]

Update LIT and fix linter issue

[fdeazeve]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

805

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]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

805

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

[fdeazeve]

Rebased

[fdeazeve]

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

[tpopp]

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.

[fdeazeve]

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]

No problem. https://reviews.llvm.org/D94132 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).

[mehdi_amini]

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

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

 
 // DEF:      void AOp::aAttrAttr(some-attr-kind attr) {
 // DEF-NEXT:   (*this)->setAttr("aAttr", attr);
 // DEF:      void AOp::bAttrAttr(some-attr-kind attr) {
 // DEF-NEXT:   (*this)->setAttr("bAttr", attr);
 // DEF:      void AOp::cAttrAttr(some-attr-kind attr) {
 // DEF-NEXT:   (*this)->setAttr("cAttr", attr);
 
+// Test remove methods
+// ---
+
+// DEF: Attribute AOp::removeCAttrAttr() {
+// DEF-NEXT: return (*this)->removeAttr("cAttr");
+
 // Test build methods
 // ---
 
 // DEF:      void AOp::build(
 // DEF:        odsState.addAttribute("aAttr", aAttr);
 // DEF:        odsState.addAttribute("bAttr", bAttr);
 // DEF:        if (cAttr) {
 // DEF-NEXT:     odsState.addAttribute("cAttr", cAttr);

 def UnitAttrOp : NS_Op<"unit_attr_op", []> {
   let arguments = (ins UnitAttr:$attr);
 }
 
 // DEF-LABEL: UnitAttrOp definitions
 // DEF: bool UnitAttrOp::attr() {
 // DEF:   return {{.*}} != nullptr
 
+// DEF: Attribute UnitAttrOp::removeAttrAttr() {
+// DEF-NEXT:   (*this)->removeAttr("attr");
+
 // DEF: build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, /*optional*/::mlir::UnitAttr attr)
 
 
 // Test elementAttr field of TypedArrayAttr.
 // ---
 
 def SomeTypedArrayAttr : TypedArrayAttrBase<SomeAttr, "SomeAttr array">;
 

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

 // CHECK:   ::mlir::Operation::result_range getODSResults(unsigned index);
 // CHECK:   ::mlir::Value r();
 // CHECK:   ::mlir::Region &someRegion();
 // CHECK:   ::mlir::MutableArrayRef<Region> someRegions();
 // CHECK:   ::mlir::IntegerAttr attr1Attr()
 // CHECK:   uint32_t attr1();
 // CHECK:   ::mlir::FloatAttr attr2Attr()
 // CHECK:   ::llvm::Optional< ::llvm::APFloat > attr2();
+// CHECK:   Attribute removeAttr2Attr();
 // CHECK:   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, Value val);
 // CHECK:   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, int integer = 0);
 // CHECK:   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type r, ::mlir::TypeRange s, ::mlir::Value a, ::mlir::ValueRange b, ::mlir::IntegerAttr attr1, /*optional*/::mlir::FloatAttr attr2, unsigned someRegionsCount)
 // CHECK:   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type r, ::mlir::TypeRange s, ::mlir::Value a, ::mlir::ValueRange b, uint32_t attr1, /*optional*/::mlir::FloatAttr attr2, unsigned someRegionsCount)
 // CHECK:   static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes, unsigned numRegions)
 // CHECK:   static ::mlir::ParseResult parse(::mlir::OpAsmParser &parser, ::mlir::OperationState &result);
 // CHECK:   void print(::mlir::OpAsmPrinter &p);
 // CHECK:   ::mlir::LogicalResult verify();

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

private:
   void genOpNameGetter();
 
   // Generates getters for the attributes.
   void genAttrGetters();
 
   // Generates setter for the attributes.
   void genAttrSetters();
 
+  // Generates removers for optional attributes.
+  void genOptionalAttrRemovers();
+
   // Generates getters for named operands.
   void genNamedOperandGetters();
 
   // Generates setters for named operands.
   void genNamedOperandSetters();
 
   // Generates getters for named results.
   void genNamedResultGetters();
OpEmitter::OpEmitter(const Operator &op,
   genOpNameGetter();
   genNamedOperandGetters();
   genNamedOperandSetters();
   genNamedResultGetters();
   genNamedRegionGetters();
   genNamedSuccessorGetters();
   genAttrGetters();
   genAttrSetters();
+  genOptionalAttrRemovers();
   genBuilder();
   genParser();
   genPrinter();
   genVerifier();
   genCanonicalizerDecls();
   genFolderDecls();
   genTypeInterfaceMethods();
   genOpInterfaceMethods();
void OpEmitter::genAttrSetters() {
   for (auto &namedAttr : op.getAttributes()) {
     const auto &name = namedAttr.name;
     const auto &attr = namedAttr.attr;
     if (!attr.isDerivedAttr())
       emitAttrWithStorageType(name, attr);
   }
 }
 
+void OpEmitter::genOptionalAttrRemovers() {
+  // Generate methods for removing optional attributes, instead of having to
+  // use the string interface. Enables better compile time verification.
+  auto emitRemoveAttr = [&](StringRef name) {
+    auto upperInitial = name.take_front().upper();
+    auto suffix = name.drop_front();
+    auto *method = opClass.addMethodAndPrune(
+        "Attribute", ("remove" + upperInitial + suffix + "Attr").str());
+    if (!method)
+      return;
+    auto &body = method->body();
+    body << "  return (*this)->removeAttr(\"" << name << "\");";
+  };
+
+  for (const auto &namedAttr : op.getAttributes()) {
+    const auto &name = namedAttr.name;
+    const auto &attr = namedAttr.attr;
+    if (attr.isOptional())
+      emitRemoveAttr(name);
+  }
+}
+
 // Generates the code to compute the start and end index of an operand or result
 // range.
 template <typename RangeT>
 static void
 generateValueRangeStartAndEnd(Class &opClass, StringRef methodName,
                               int numVariadic, int numNonVariadic,
                               StringRef rangeSizeCall, bool hasAttrSegmentSize,
                               StringRef sizeAttrInit, RangeT &&odsValues) {