Add sanity check in MLIR ODS to catch case where an arguments/results/regions/successors names overlap

[mehdi_amini]

Summary

This is making a tablegen crash with a more friendly error.

[rriddle]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

478

Should this be named assertNameInvariants? Or do we plan on adding more checks here?

479

Could we use something like DenseMap<StringRef, StringRef>? Where the value of the dense map would be the type of the existing entity? That way we could construct one message that covers all possible overlap cases, i.e. something like:

DenseMap<StringRef, StringRef> existingNames;
auto recordName = [&](StringRef name, StringRef type) {
  if (name.empty())
    return;
  StringRef &nameType = existingNames[name];
  if (!nameType.empty()) {
      if (nameType == type) {
        PrintFatalError(
            op.getLoc(), llvm::formatv(
            "op has two {0}s with the same name: '{1}'", nameType, name));
      } else {
        PrintFatalError(
            op.getLoc(), llvm::formatv(
            "op is using the same name for a {0} and a {1}: '{2}'", nameType, type, name));
      }
  }
  nameType = type;
};

for (int i : llvm::seq<int>(0, op.getNumOperands()))
    recordName(op.getOperand(i).name, "operand");
...

[mehdi_amini]

Move the invariant checking on the Operator class construction and check for region/successor overlap as well

[mehdi_amini]

Refactor / code reduction

[mehdi_amini]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

478

I thought I'd leave it open to more invariants as we see fit?

[mehdi_amini]

Update title

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

 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR1 %s 2>&1 | FileCheck --check-prefix=ERROR1 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR2 %s 2>&1 | FileCheck --check-prefix=ERROR2 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR3 %s 2>&1 | FileCheck --check-prefix=ERROR3 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR4 %s 2>&1 | FileCheck --check-prefix=ERROR4 %s
 // RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR5 %s 2>&1 | FileCheck --check-prefix=ERROR5 %s
+// RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR6 %s 2>&1 | FileCheck --check-prefix=ERROR6 %s
 
 include "mlir/IR/OpBase.td"
 
 def Test_Dialect : Dialect {
   let name = "test_dialect";
 }
 
 #ifdef ERROR1

 #endif
 
 #ifdef ERROR5
 // ERROR5: error: op has two results with the same name: 'tensor'
 def OpWithDuplicatedResultNames : Op<Test_Dialect, "default_value"> {
   let results = (outs AnyTensor:$tensor, AnyTensor:$tensor);
 }
 #endif
+
+#ifdef ERROR6
+// ERROR6: error: op is using the same name for an operand and a result: 'tensor'
+def OpWithDuplicatedArgResultNames : Op<Test_Dialect, "default_value"> {
+  let arguments = (ins AnyTensor:$tensor);
+  let results = (outs AnyTensor:$tensor);
+}
+#endif

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

OpMethod *genOpInterfaceMethod(const tblgen::InterfaceMethod &method,
                                  bool declaration = true);
 
   // Generate the side effect interface methods.
   void genSideEffectInterfaceMethods();
 
   // Generate the type inference interface methods.
   void genTypeInterfaceMethods();
 
+  // Verify the op invariants that emitters may rely on.
+  void assertInvariants();
+
 private:
   // The TableGen record for this op.
   // TODO: OpEmitter should not have a Record directly,
   // it should rather go through the Operator for better abstraction.
   const Record &def;
 
   // The wrapper operator class for querying information from this op.
   Operator op;
body << tgfmt("    if (!($0)) return $1\"attribute '$2' "
                   /*ctx=*/nullptr, tgfmt(condition, &ctx.withSelf(varName)),
                   emitErrorPrefix, attrName, attr.getSummary());
     if (allowMissingAttr)
       body << "  }\n";
     body << "  }\n";
   }
 }
 
+void OpEmitter::assertInvariants() {
+  SmallDenseSet<StringRef> operandNames;
+  for (int i : llvm::seq<int>(0, op.getNumOperands())) {
+    const auto &operand = op.getOperand(i);
+    if (operand.name.empty())
+      continue;
+    if (!operandNames.insert(operand.name).second)
+      PrintFatalError(op.getLoc(), "op has two operands with the same name: '" +
+                                       operand.name + "'");
+  }
+  SmallDenseSet<StringRef> resultNames;
+  for (int i : llvm::seq<int>(0, op.getNumResults())) {
+    const auto &result = op.getResult(i);
+    if (result.name.empty())
+      continue;
+    if (!resultNames.insert(result.name).second)
+      PrintFatalError(op.getLoc(), "op has two results with the same name: '" +
+                                       result.name + "'");
+    if (operandNames.contains(result.name))
+      PrintFatalError(
+          op.getLoc(),
+          "op is using the same name for an operand and a result: '" +
+              result.name + "'");
+  }
+}
+
 OpEmitter::OpEmitter(const Operator &op,
                      const StaticVerifierFunctionEmitter &staticVerifierEmitter)
     : def(op.getDef()), op(op),
       opClass(op.getCppClassName(), op.getExtraClassDeclaration()),
       staticVerifierEmitter(staticVerifierEmitter) {
   verifyCtx.withOp("(*this->getOperation())");
   verifyCtx.addSubst("_ctxt", "this->getOperation()->getContext()");
+  assertInvariants();
 
   genTraits();
 
   // Generate C++ code for various op methods. The order here determines the
   // methods in the generated file.
   genAttrNameGetters();
   genOpAsmInterface();
   genOpNameGetter();
static void generateNamedOperandGetters(const Operator &op, Class &opClass,
   auto *m = opClass.addMethodAndPrune(rangeType, "getODSOperands", "unsigned",
                                       "index");
   auto &body = m->body();
   body << formatv(valueRangeReturnCode, rangeBeginCall,
                   "getODSOperandIndexAndLength(index)");
 
   // Then we emit nicer named getter methods by redirecting to the "sink" getter
   // method.
-  // Keep track of the operand names to find duplicates.
-  SmallDenseSet<StringRef> operandNames;
   for (int i = 0; i != numOperands; ++i) {
     const auto &operand = op.getOperand(i);
     if (operand.name.empty())
       continue;
-    if (!operandNames.insert(operand.name).second)
-      PrintFatalError(op.getLoc(), "op has two operands with the same name: '" +
-                                       operand.name + "'");
-
     if (operand.isOptional()) {
       m = opClass.addMethodAndPrune("::mlir::Value", operand.name);
       m->body()
           << "  auto operands = getODSOperands(" << i << ");\n"
           << "  return operands.empty() ? ::mlir::Value() : *operands.begin();";
     } else if (operand.isVariadicOfVariadic()) {
       StringRef segmentAttr =
           operand.constraint.getVariadicOfVariadicSegmentSizeAttr();
generateValueRangeStartAndEnd(
       numNormalResults, "getOperation()->getNumResults()", attrSizedResults,
       attrSizeInitCode, op.getResults());
 
   auto *m = opClass.addMethodAndPrune("::mlir::Operation::result_range",
                                       "getODSResults", "unsigned", "index");
   m->body() << formatv(valueRangeReturnCode, "getOperation()->result_begin()",
                        "getODSResultIndexAndLength(index)");
 
-  SmallDenseSet<StringRef> resultNames;
   for (int i = 0; i != numResults; ++i) {
     const auto &result = op.getResult(i);
     if (result.name.empty())
       continue;
-    if (!resultNames.insert(result.name).second)
-      PrintFatalError(op.getLoc(), "op has two results with the same name: '" +
-                                       result.name + "'");
-
     if (result.isOptional()) {
       m = opClass.addMethodAndPrune("::mlir::Value", result.name);
       m->body()
           << "  auto results = getODSResults(" << i << ");\n"
           << "  return results.empty() ? ::mlir::Value() : *results.begin();";
     } else if (result.isVariadic()) {
       m = opClass.addMethodAndPrune("::mlir::Operation::result_range",
                                     result.name);