[mlir][PDLL] Don't use the result of `Constraint::getDefName()` when uniquing

[rriddle]

Summary

In the case of anonymous defs this may return the name of the base def class,
which can lead to two different defs with the same name (which hits an assert).
This commit adds a new getUniqueDefName method that returns a unique name
for the constraint.

[jpienaar]

mlir/lib/Tools/PDLL/ODS/Constraint.cpp

25

Shouldn't this be drop_front?

[rriddle]

mlir/lib/Tools/PDLL/ODS/Constraint.cpp

25

No, the names are usually in the form prettyBaseName(anonymous_...). We want to strip the anonymous stuff off of the end, so we take everything from the front up to the (. The prettyBaseName is what we want to display as the "pretty"/demangled name.

mlir/include/mlir/TableGen/Constraint.h

public:
   // description is not provided, returns the TableGen def name.
   StringRef getSummary() const;
 
   /// Returns the name of the TablGen def of this constraint. In some cases
   /// where the current def is anonymous, the name of the base def is used (e.g.
   /// `Optional<>`/`Variadic<>` type constraints).
   StringRef getDefName() const;
 
+  /// Returns a unique name for the TablGen def of this constraint. This is
+  /// generally just the name of the def, but in some cases where the current
+  /// def is anonymous, the name of the base def is attached (to provide more
+  /// context on the def).
+  std::string getUniqueDefName() const;
+
   Kind getKind() const { return kind; }
 
 protected:
   // The TableGen definition of this constraint.
   const llvm::Record *def;
 
 private:
+  /// Return the name of the base def if there is one, or None otherwise.
+  Optional<StringRef> getBaseDefName() const;
+
   // What kind of constraint this is.
   Kind kind;
 };
 
 // An constraint and the concrete entities to place the constraint on.
 struct AppliedConstraint {
   AppliedConstraint(Constraint &&constraint, StringRef self,
                     std::vector<std::string> &&entities);

mlir/include/mlir/Tools/PDLL/ODS/Constraint.h

 
 //===----------------------------------------------------------------------===//
 // Constraint
 //===----------------------------------------------------------------------===//
 
 /// This class represents a generic ODS constraint.
 class Constraint {
 public:
-  /// Return the name of this constraint.
+  /// Return the unique name of this constraint.
   StringRef getName() const { return name; }
 
+  /// Return the demangled name of this constraint. This tries to strip out bits
+  /// of the name that are purely for uniquing, and show the underlying name. As
+  /// such, this name does guarantee uniqueness and should only be used for
+  /// logging or other lossy friendly "pretty" output.
+  StringRef getDemangledName() const;
+
   /// Return the summary of this constraint.
   StringRef getSummary() const { return summary; }
 
 protected:
   Constraint(StringRef name, StringRef summary)
       : name(name.str()), summary(summary.str()) {}
   Constraint(const Constraint &) = delete;
 

mlir/lib/TableGen/Constraint.cpp

 
 StringRef Constraint::getSummary() const {
   if (Optional<StringRef> summary = def->getValueAsOptionalString("summary"))
     return *summary;
   return def->getName();
 }
 
 StringRef Constraint::getDefName() const {
+  if (Optional<StringRef> baseDefName = getBaseDefName())
+    return *baseDefName;
+  return def->getName();
+}
+
+std::string Constraint::getUniqueDefName() const {
+  std::string defName = def->getName().str();
+
+  // Non-anonymous classes already have a unique name from the def.
+  if (!def->isAnonymous())
+    return defName;
+
+  // Otherwise, this is an anonymous class. In these cases we still use the def
+  // name, but we also try attach the name of the base def when present to make
+  // the name more obvious.
+  if (Optional<StringRef> baseDefName = getBaseDefName())
+    return (*baseDefName + "(" + defName + ")").str();
+  return defName;
+}
+
+Optional<StringRef> Constraint::getBaseDefName() const {
   // Functor used to check a base def in the case where the current def is
   // anonymous.
-  auto checkBaseDefFn = [&](StringRef baseName) {
-    if (const auto *init = dyn_cast<llvm::DefInit>(def->getValueInit(baseName)))
-      return Constraint(init->getDef(), kind).getDefName();
-    return def->getName();
+  auto checkBaseDefFn = [&](StringRef baseName) -> Optional<StringRef> {
+    if (const auto *defValue = def->getValue(baseName)) {
+      if (const auto *defInit = dyn_cast<llvm::DefInit>(defValue->getValue()))
+        return Constraint(defInit->getDef(), kind).getDefName();
+    }
+    return llvm::None;
   };
 
   switch (kind) {
   case CK_Attr:
     if (def->isAnonymous())
       return checkBaseDefFn("baseAttr");
-    return def->getName();
+    return llvm::None;
   case CK_Type:
     if (def->isAnonymous())
       return checkBaseDefFn("baseType");
-    return def->getName();
+    return llvm::None;
   default:
-    return def->getName();
+    return llvm::None;
   }
 }
 
 AppliedConstraint::AppliedConstraint(Constraint &&constraint,
                                      llvm::StringRef self,
                                      std::vector<std::string> &&entities)
     : constraint(constraint), self(std::string(self)),
       entities(std::move(entities)) {}

mlir/lib/Tools/PDLL/ODS/CMakeLists.txt

 add_mlir_library(MLIRPDLLODS
+  Constraint.cpp
   Context.cpp
   Dialect.cpp
   Operation.cpp
 
   LINK_LIBS PUBLIC
   MLIRSupport
   )

mlir/lib/Tools/PDLL/ODS/Constraint.cpp

+//===- Constraint.cpp -----------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "mlir/Tools/PDLL/ODS/Constraint.h"
+
+using namespace mlir;
+using namespace mlir::pdll::ods;
+
+//===----------------------------------------------------------------------===//
+// Constraint
+//===----------------------------------------------------------------------===//
+
+StringRef Constraint::getDemangledName() const {
+  StringRef demangledName = name;
+
+  // Drop the "anonymous" suffix if present.
+  size_t anonymousSuffix = demangledName.find("(anonymous_");
+  if (anonymousSuffix != StringRef::npos)
+    demangledName = demangledName.take_front(anonymousSuffix);
+  return demangledName;
+}

mlir/lib/Tools/PDLL/ODS/Context.cpp

for (const Operation *op : sortMapByName(dialect->getOperations())) {
       ArrayRef<Attribute> attributes = op->getAttributes();
       if (!attributes.empty()) {
         printer.startLine() << "Attributes { ";
         llvm::interleaveComma(attributes, os, [&](const Attribute &attr) {
           os << attr.getName() << " : ";
 
           auto kind = attr.isOptional() ? VariableLengthKind::Optional
                                         : VariableLengthKind::Single;
-          printVariableLengthCst(attr.getConstraint().getName(), kind);
+          printVariableLengthCst(attr.getConstraint().getDemangledName(), kind);
         });
         os << " }\n";
       }
 
       // Operands.
       ArrayRef<OperandOrResult> operands = op->getOperands();
       if (!operands.empty()) {
         printer.startLine() << "Operands { ";
         llvm::interleaveComma(
             operands, os, [&](const OperandOrResult &operand) {
               os << operand.getName() << " : ";
-              printVariableLengthCst(operand.getConstraint().getName(),
+              printVariableLengthCst(operand.getConstraint().getDemangledName(),
                                      operand.getVariableLengthKind());
             });
         os << " }\n";
       }
 
       // Results.
       ArrayRef<OperandOrResult> results = op->getResults();
       if (!results.empty()) {
         printer.startLine() << "Results { ";
         llvm::interleaveComma(results, os, [&](const OperandOrResult &result) {
           os << result.getName() << " : ";
-          printVariableLengthCst(result.getConstraint().getName(),
+          printVariableLengthCst(result.getConstraint().getDemangledName(),
                                  result.getVariableLengthKind());
         });
         os << " }\n";
       }
 
       printer.objectEnd();
     }
     printer.objectEnd();
   }
   for (const AttributeConstraint *cst : sortMapByName(attributeConstraints)) {
-    printer.startLine() << "AttributeConstraint `" << cst->getName() << "` {\n";
+    printer.startLine() << "AttributeConstraint `" << cst->getDemangledName()
+                        << "` {\n";
     printer.indent();
 
     printer.startLine() << "Summary: " << cst->getSummary() << "\n";
     printer.startLine() << "CppClass: " << cst->getCppClass() << "\n";
     printer.objectEnd();
   }
   for (const TypeConstraint *cst : sortMapByName(typeConstraints)) {
-    printer.startLine() << "TypeConstraint `" << cst->getName() << "` {\n";
+    printer.startLine() << "TypeConstraint `" << cst->getDemangledName()
+                        << "` {\n";
     printer.indent();
 
     printer.startLine() << "Summary: " << cst->getSummary() << "\n";
     printer.startLine() << "CppClass: " << cst->getCppClass() << "\n";
     printer.objectEnd();
   }
   printer.objectEnd();
 }

mlir/lib/Tools/PDLL/Parser/Parser.cpp

auto getLengthKind = [](const auto &value) {
     return value.isVariadic() ? ods::VariableLengthKind::Variadic
                               : ods::VariableLengthKind::Single;
   };
 
   // Insert a type constraint into the ODS context.
   ods::Context &odsContext = ctx.getODSContext();
   auto addTypeConstraint = [&](const tblgen::NamedTypeConstraint &cst)
       -> const ods::TypeConstraint & {
-    return odsContext.insertTypeConstraint(cst.constraint.getDefName(),
+    return odsContext.insertTypeConstraint(cst.constraint.getUniqueDefName(),
                                            cst.constraint.getSummary(),
                                            cst.constraint.getCPPClassName());
   };
   auto convertLocToRange = [&](llvm::SMLoc loc) -> llvm::SMRange {
     return {loc, llvm::SMLoc::getFromPointer(loc.getPointer() + 1)};
   };
 
   // Process the parsed tablegen records to build ODS information.
for (llvm::Record *def : tdRecords.getAllDerivedDefinitions("Op")) {
 
     // Ignore operations that have already been added.
     if (!inserted)
       continue;
 
     for (const tblgen::NamedAttribute &attr : op.getAttributes()) {
       odsOp->appendAttribute(
           attr.name, attr.attr.isOptional(),
-          odsContext.insertAttributeConstraint(attr.attr.getAttrDefName(),
+          odsContext.insertAttributeConstraint(attr.attr.getUniqueDefName(),
                                                attr.attr.getSummary(),
                                                attr.attr.getStorageType()));
     }
     for (const tblgen::NamedTypeConstraint &operand : op.getOperands()) {
       odsOp->appendOperand(operand.name, getLengthKind(operand),
                            addTypeConstraint(operand));
     }
     for (const tblgen::NamedTypeConstraint &result : op.getResults()) {

 Parser::createODSNativePDLLConstraintDecl(const tblgen::Constraint &constraint,
                                           SMRange loc, ast::Type type) {
   // Format the condition template.
   tblgen::FmtContext fmtContext;
   fmtContext.withSelf("self");
   std::string codeBlock =
       tblgen::tgfmt(constraint.getConditionTemplate(), &fmtContext);
 
-  return createODSNativePDLLConstraintDecl<ConstraintT>(constraint.getDefName(),
-                                                        codeBlock, loc, type);
+  return createODSNativePDLLConstraintDecl<ConstraintT>(
+      constraint.getUniqueDefName(), codeBlock, loc, type);
 }
 
 //===----------------------------------------------------------------------===//
 // Decls
 
 FailureOr<ast::Decl *> Parser::parseTopLevelDecl() {
   FailureOr<ast::Decl *> decl;
   switch (curToken.getKind()) {

mlir/test/mlir-pdll/Parser/include/ops.td

 include "include/interfaces.td"
 
 def Test_Dialect : Dialect {
   let name = "test";
 }
 
 def OpAllEmpty : Op<Test_Dialect, "all_empty">;
 
 def OpAllSingle : Op<Test_Dialect, "all_single"> {
-  let arguments = (ins I64:$operand, I64Attr:$attr);
+  let arguments = (ins
+    I64:$operand,
+    I64Attr:$attr,
+    Confined<I64Attr, [IntNonNegative]>:$nonNegativeAttr
+  );
   let results = (outs I64:$result);
 }
 
 def OpAllOptional : Op<Test_Dialect, "all_optional"> {
   let arguments = (ins Optional<I64>:$operand, OptionalAttr<I64Attr>:$attr);
   let results = (outs Optional<I64>:$result);
 }
 
 def OpAllVariadic : Op<Test_Dialect, "all_variadic"> {
   let arguments = (ins Variadic<I64>:$operands);
   let results = (outs Variadic<I64>:$results);
 }
 
 def OpMultipleSingleResult : Op<Test_Dialect, "multiple_single_result"> {
   let results = (outs I64:$result, I64:$result2);
 }

mlir/test/mlir-pdll/Parser/include_td.pdll

 // RUN: mlir-pdll %s -I %S -I %S/../../../include -dump-ods 2>&1 | FileCheck %s
 
 #include "include/ops.td"
 
 // CHECK:      Operation `test.all_empty` {
 // CHECK-NEXT: }
 
 // CHECK:      Operation `test.all_optional` {
 // CHECK-NEXT:   Attributes { attr : Optional<I64Attr> }
 // CHECK-NEXT:   Operands { operand : Optional<I64> }
 // CHECK-NEXT:   Results { result : Optional<I64> }
 // CHECK-NEXT: }
 
 // CHECK:      Operation `test.all_single` {
-// CHECK-NEXT:   Attributes { attr : I64Attr }
+// CHECK-NEXT:   Attributes { attr : I64Attr, nonNegativeAttr : I64Attr }
 // CHECK-NEXT:   Operands { operand : I64 }
 // CHECK-NEXT:   Results { result : I64 }
 // CHECK-NEXT: }
 
 // CHECK:      Operation `test.all_variadic` {
 // CHECK-NEXT:   Operands { operands : Variadic<I64> }
 // CHECK-NEXT:   Results { results : Variadic<I64> }
 // CHECK-NEXT: }