From 7fb2394a4f362aff4282af8486b7352e720c32ab Mon Sep 17 00:00:00 2001 From: Mehdi Amini Date: Thu, 9 Sep 2021 00:10:16 +0000 Subject: [PATCH] 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 | 4 ++ mlir/lib/TableGen/Operator.cpp | 36 ++++++++++++++++++ mlir/test/mlir-tblgen/op-error.td | 58 ++++++++++++++++++++++++++++- mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp | 11 ------ 4 files changed, 96 insertions(+), 13 deletions(-) diff --git a/mlir/include/mlir/TableGen/Operator.h b/mlir/include/mlir/TableGen/Operator.h index 4d41d48..69e76d3 100644 --- a/mlir/include/mlir/TableGen/Operator.h +++ b/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 { diff --git a/mlir/lib/TableGen/Operator.cpp b/mlir/lib/TableGen/Operator.cpp index 03e5170..57c957f 100644 --- a/mlir/lib/TableGen/Operator.cpp +++ b/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 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(0, getNumOperands())) + checkName(getOperand(i).name, "operands"); + + // Check results amongst themselves and against operands. + for (int i : llvm::seq(0, getNumResults())) + checkName(getResult(i).name, "results"); + + // Check regions amongst themselves and against operands and results. + for (int i : llvm::seq(0, getNumRegions())) + checkName(getRegion(i).name, "regions"); + + // Check successors amongst themselves and against operands, results, and + // regions. + for (int i : llvm::seq(0, getNumSuccessors())) + checkName(getSuccessor(i).name, "successors"); +} + StringRef Operator::getDialectName() const { return dialect.getName(); } StringRef Operator::getCppClassName() const { return cppClassName; } diff --git a/mlir/test/mlir-tblgen/op-error.td b/mlir/test/mlir-tblgen/op-error.td index 587738e..4be846b 100644 --- a/mlir/test/mlir-tblgen/op-error.td +++ b/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 { #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 { 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 { 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 { + 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 { + 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 { + 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 { + 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 { + 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 { + let successors = (successor AnySuccessor:$target); + let regions = (region AnyRegion:$target); +} +#endif diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp index b9d63dc..e40fa9b 100644 --- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp +++ b/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 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 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() -- 2.7.4