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

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

Differential Revision: https://reviews.llvm.org/D109474

mlir/include/mlir/TableGen/Operator.h

@@ -64,6 +64,10 @@ public:
   // Returns the name of op's adaptor C++ class.
   std::string getAdaptorName() const;
 
+  // Check invariants (like no duplicated or conflicted names) and abort the
+  // process if any invariant is broken.
+  void assertInvariants() const;
+
   /// A class used to represent the decorators of an operator variable, i.e.
   /// argument or result.
   struct VariableDecorator {

mlir/lib/TableGen/Operator.cpp

@@ -53,6 +53,7 @@ Operator::Operator(const llvm::Record &def)
   cppNamespace = def.getValueAsString("cppNamespace");
 
   populateOpStructure();
+  assertInvariants();
 }
 
 std::string Operator::getOperationName() const {
@@ -67,6 +68,41 @@ std::string Operator::getAdaptorName() const {
   return std::string(llvm::formatv("{0}Adaptor", getCppClassName()));
 }
 
+void Operator::assertInvariants() const {
+  // Check that the name of arguments/results/regions/successors don't overlap.
+  DenseMap<StringRef, StringRef> existingNames;
+  auto checkName = [&](StringRef name, StringRef entity) {
+    if (name.empty())
+      return;
+    auto insertion = existingNames.insert({name, entity});
+    if (insertion.second)
+      return;
+    if (entity == insertion.first->second)
+      PrintFatalError(getLoc(), "op has a conflict with two " + entity +
+                                    " having the same name '" + name + "'");
+    PrintFatalError(getLoc(), "op has a conflict with " +
+                                  insertion.first->second + " and " + entity +
+                                  " both having an entry with the name '" +
+                                  name + "'");
+  };
+  // Check operands amongst themselves.
+  for (int i : llvm::seq<int>(0, getNumOperands()))
+    checkName(getOperand(i).name, "operands");
+
+  // Check results amongst themselves and against operands.
+  for (int i : llvm::seq<int>(0, getNumResults()))
+    checkName(getResult(i).name, "results");
+
+  // Check regions amongst themselves and against operands and results.
+  for (int i : llvm::seq<int>(0, getNumRegions()))
+    checkName(getRegion(i).name, "regions");
+
+  // Check successors amongst themselves and against operands, results, and
+  // regions.
+  for (int i : llvm::seq<int>(0, getNumSuccessors()))
+    checkName(getSuccessor(i).name, "successors");
+}
+
 StringRef Operator::getDialectName() const { return dialect.getName(); }
 
 StringRef Operator::getCppClassName() const { return cppClassName; }

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

@@ -3,6 +3,12 @@
 // 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
+// RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR7 %s 2>&1 | FileCheck --check-prefix=ERROR7 %s
+// RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR8 %s 2>&1 | FileCheck --check-prefix=ERROR8 %s
+// RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR9 %s 2>&1 | FileCheck --check-prefix=ERROR9 %s
+// RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR10 %s 2>&1 | FileCheck --check-prefix=ERROR10 %s
+// RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR11 %s 2>&1 | FileCheck --check-prefix=ERROR11 %s
 
 include "mlir/IR/OpBase.td"
 
@@ -38,15 +44,63 @@ def OpDefaultValueNotTrailing : Op<Test_Dialect, "default_value"> {
 #endif
 
 #ifdef ERROR4
-// ERROR4: error: op has two operands with the same name: 'tensor'
+// ERROR4: error: op has a conflict with two operands having the same name 'tensor'
 def OpWithDuplicatedArgNames : Op<Test_Dialect, "default_value"> {
   let arguments = (ins AnyTensor:$tensor, AnyTensor:$tensor);
 }
 #endif
 
 #ifdef ERROR5
-// ERROR5: error: op has two results with the same name: 'tensor'
+// ERROR5: error: op has a conflict with two results having the same name 'tensor'
 def OpWithDuplicatedResultNames : Op<Test_Dialect, "default_value"> {
   let results = (outs AnyTensor:$tensor, AnyTensor:$tensor);
 }
 #endif
+
+#ifdef ERROR6
+// ERROR6: error: op has a conflict with operands and results both having an entry with the name 'tensor'
+def OpWithDuplicatedArgResultNames : Op<Test_Dialect, "default_value"> {
+  let arguments = (ins AnyTensor:$tensor);
+  let results = (outs AnyTensor:$tensor);
+}
+#endif
+
+#ifdef ERROR7
+// ERROR7: error: op has a conflict with operands and regions both having an entry with the name 'tensor'
+def OpWithDuplicatedArgResultNames : Op<Test_Dialect, "default_value"> {
+  let arguments = (ins AnyTensor:$tensor);
+  let regions = (region AnyRegion:$tensor);
+}
+#endif
+
+#ifdef ERROR8
+// ERROR8: error: op has a conflict with results and regions both having an entry with the name 'tensor'
+def OpWithDuplicatedArgResultNames : Op<Test_Dialect, "default_value"> {
+  let results = (outs AnyTensor:$tensor);
+  let regions = (region AnyRegion:$tensor);
+}
+#endif
+
+#ifdef ERROR9
+// ERROR9: error: op has a conflict with operands and successors both having an entry with the name 'target'
+def OpWithDuplicatedArgResultNames : Op<Test_Dialect, "default_value"> {
+  let successors = (successor AnySuccessor:$target);
+  let arguments = (ins AnyTensor:$target);
+}
+#endif
+
+#ifdef ERROR10
+// ERROR10: error: op has a conflict with results and successors both having an entry with the name 'target'
+def OpWithDuplicatedArgResultNames : Op<Test_Dialect, "default_value"> {
+  let successors = (successor AnySuccessor:$target);
+  let results = (outs AnyTensor:$target);
+}
+#endif
+
+#ifdef ERROR11
+// ERROR11: error: op has a conflict with regions and successors both having an entry with the name 'target'
+def OpWithDuplicatedArgResultNames : Op<Test_Dialect, "default_value"> {
+  let successors = (successor AnySuccessor:$target);
+  let regions = (region AnyRegion:$target);
+}
+#endif

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

@@ -850,16 +850,10 @@ static void generateNamedOperandGetters(const Operator &op, Class &opClass,
 
   // 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()
@@ -992,15 +986,10 @@ void OpEmitter::genNamedResultGetters() {
   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()