[mlir] Printing oilist element

[shraiysh]

Summary

This patch attempts to deduce when the oilist element must be printed
based on the optional arguments to it. This especially helps creating
an operation accurately because with the current implementation, the
inferred unit attributes must be manually added to print the clauses
appropriately.

[shraiysh]

Formatting.

[rriddle]

Missing test cases?

[shraiysh]

In D121579#3379996, @rriddle wrote:

Missing test cases?

Where should I add the testcases for this? This is basically an alternative implementation of printing (tests to check printing are already in the repo - mlir/test/IR/traits.mlir - they act as tests for this patch too). The builders will be benefiting from this implementation as they no longer will have to add the inferred attribute. I could not find a test file for testing builder functions or to check the generated C/C++ code for operations, but please let me know if there is one. (AFAICT mlir/test/mlir-tblgen/attr-or-type-format.td checks C++ code generated for attributes and types only).

[shraiysh]

Added handling for unit attributes. Now no extra attributes are added because of the oilist element.

[kiranchandramohan]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

1200

Nit: Curlies are probably required here. https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

1892

Nit: Curlies are probably not required here and in the other single line for statements.

2970

Nit: Braces not required.

[shraiysh]

Addressed comments.

[Mogball]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

1200

spell out the auto

1987

drop trivial braces (we should probably have a formatter for this..)

1991

spell out the auto

1998

add braces

2005

add braces

[shraiysh]

Addressed comments, rebase with main.

mlir/tools/mlir-tblgen/OpFormatGen.cpp

for (auto clause : oilist->getClauses()) {
       genLiteralParser(lelement->getSpelling(), body);
       body << ")) {\n";
       StringRef attrName = lelement->getSpelling();
       body << formatv(oilistParserCode, attrName);
       inferredAttributes.insert(attrName);
       for (FormatElement *el : pelement)
         genElementParser(el, body, attrTypeCtx);
       body << "    } else ";
     }
     body << " {\n";
     body << "    break;\n";
     body << "  }\n";
     body << "}\n";
 
     /// Literals.
   } else if (LiteralElement *literal = dyn_cast<LiteralElement>(element)) {
     body << "  if (parser.parse";
TypeSwitch<FormatElement *>(anchor)
         genOptionalGroupPrinterAnchor(element->getInputs(), op, body);
       })
       .Case<AttributeVariable>([&](AttributeVariable *attr) {
         body << "  if ((*this)->getAttr(\"" << attr->getVar()->name
              << "\")) {\n";
       });
 }
 
+void collect(FormatElement *element,
+             SmallVectorImpl<VariableElement *> &variables) {
+  TypeSwitch<FormatElement *>(element)
+      .Case([&](VariableElement *var) { variables.emplace_back(var); })
+      .Case([&](CustomDirective *ele) {
+        for (FormatElement *arg : ele->getArguments()) {
+          collect(arg, variables);
+        }
+      })
+      .Case([&](OptionalElement *ele) {
+        for (FormatElement *arg : ele->getThenElements()) {
+          collect(arg, variables);
+        }
+        for (FormatElement *arg : ele->getElseElements()) {
+          collect(arg, variables);
+        }
+      })
+      .Case([&](FunctionalTypeDirective *funcType) {
+        collect(funcType->getInputs(), variables);
+        collect(funcType->getResults(), variables);
+      })
+      .Case([&](OIListElement *oilist) {
+        for (ArrayRef<FormatElement *> arg : oilist->getParsingElements())
+          for (FormatElement *arg_ : arg)
+            collect(arg_, variables);
+      });
+}
+
 void OperationFormat::genElementPrinter(FormatElement *element,
                                         MethodBody &body, Operator &op,
                                         bool &shouldEmitSpace,
                                         bool &lastWasPunctuation) {
   if (LiteralElement *literal = dyn_cast<LiteralElement>(element))
     return genLiteralPrinter(literal->getSpelling(), body, shouldEmitSpace,
                              lastWasPunctuation);
 
void OperationFormat::genElementPrinter(FormatElement *element,
 
   // Emit the OIList
   if (auto *oilist = dyn_cast<OIListElement>(element)) {
     genLiteralPrinter(" ", body, shouldEmitSpace, lastWasPunctuation);
     for (auto clause : oilist->getClauses()) {
       LiteralElement *lelement = std::get<0>(clause);
       ArrayRef<FormatElement *> pelement = std::get<1>(clause);
 
+      SmallVector<VariableElement*> vars;
+      for (FormatElement *el : pelement) {
+        collect(el, vars);
+      }
       body << "  if ((*this)->hasAttrOfType<UnitAttr>(\""
-           << lelement->getSpelling() << "\")) {\n";
+           << lelement->getSpelling() << "\")";
+      for(auto *var : vars) {
+        TypeSwitch<FormatElement *>(var)
+            .Case([&](AttributeVariable *attrEle) {
+              body << " || " << op.getGetterName(attrEle->getVar()->name) << "Attr()";
+            })
+            .Case([&](OperandVariable *ele) {
+              if (ele->getVar()->isVariadic())
+                body << " || " << op.getGetterName(ele->getVar()->name) << "().size()";
+              else
+                body << " || " << op.getGetterName(ele->getVar()->name) << "()";
+            })
+            .Case([&](ResultVariable *ele) {
+              if(ele->getVar()->isVariadic())
+                body << " || " << op.getGetterName(ele->getVar()->name) << "().size()";
+              else
+                body << " || " << op.getGetterName(ele->getVar()->name) << "()";
+            })
+            .Case([&](RegionVariable *reg) {
+              body << " || " << op.getGetterName(reg->getVar()->name) << "()";
+            });
+      }
+
+      body << ") {\n";
       genLiteralPrinter(lelement->getSpelling(), body, shouldEmitSpace,
                         lastWasPunctuation);
       for (FormatElement *element : pelement) {
         genElementPrinter(element, body, op, shouldEmitSpace,
                           lastWasPunctuation);
       }
       body << "  }\n";
     }
OpFormatParser::parseOIListDirective(SMLoc loc, Context context) {
   } while (true);
 
   return create<OIListElement>(std::move(literalElements),
                                std::move(parsingElements));
 }
 
 LogicalResult OpFormatParser::verifyOIListParsingElement(FormatElement *element,
                                                          SMLoc loc) {
-  return TypeSwitch<FormatElement *, LogicalResult>(element)
+  SmallVector<VariableElement*> vars;
+  collect(element, vars);
+  for(VariableElement *elem: vars) {
+    LogicalResult res =
+        TypeSwitch<FormatElement *, LogicalResult>(elem)
             // Only optional attributes can be within an oilist parsing group.
             .Case([&](AttributeVariable *attrEle) {
-        if (!attrEle->getVar()->attr.isOptional())
-          return emitError(loc, "only optional attributes can be used to "
-                                "in an oilist parsing group");
+              if (!attrEle->getVar()->attr.isOptional() &&
+                  !attrEle->getVar()->attr.hasDefaultValue())
+                return emitError(loc, "only optional attributes can be used in "
+                                      "an oilist parsing group");
               return success();
             })
-      // Only optional-like(i.e. variadic) operands can be within an oilist
-      // parsing group.
+            // Only optional-like(i.e. variadic) operands can be within an
+            // oilist parsing group.
             .Case([&](OperandVariable *ele) {
               if (!ele->getVar()->isVariableLength())
                 return emitError(loc, "only variable length operands can be "
                                       "used within an oilist parsing group");
               return success();
             })
             // Only optional-like(i.e. variadic) results can be within an oilist
             // parsing group.
             .Case([&](ResultVariable *ele) {
               if (!ele->getVar()->isVariableLength())
                 return emitError(loc, "only variable length results can be "
                                       "used within an oilist parsing group");
               return success();
             })
-      .Case([&](RegionVariable *) {
-        // TODO: When ODS has proper support for marking "optional" regions, add
-        // a check here.
-        return success();
-      })
-      .Case([&](TypeDirective *ele) {
-        return verifyOIListParsingElement(ele->getArg(), loc);
-      })
-      .Case([&](FunctionalTypeDirective *ele) {
-        if (failed(verifyOIListParsingElement(ele->getInputs(), loc)))
-          return failure();
-        return verifyOIListParsingElement(ele->getResults(), loc);
-      })
-      // Literals, whitespace, and custom directives may be used.
-      .Case<LiteralElement, WhitespaceElement, CustomDirective,
-            FunctionalTypeDirective, OptionalElement>(
-          [&](FormatElement *) { return success(); })
+            .Case([&](RegionVariable *) { return success(); })
             .Default([&](FormatElement *) {
-        return emitError(loc, "only literals, types, and variables can be "
+              return emitError(loc,
+                               "only literals, types, and variables can be "
                                "used within an oilist group");
             });
+    if (failed(res)) {
+      return failure();
+    }
+  }
+  return success();
 }
 
 FailureOr<FormatElement *> OpFormatParser::parseTypeDirective(SMLoc loc,
                                                               Context context) {
   if (context == TypeDirectiveContext)
     return emitError(loc, "'type' cannot be used as a child of another `type`");
 
   bool isRefChild = context == RefDirectiveContext;