[mlir-tblgen] Avoid ODS verifier duplication

[Chia-hungDuan]

Summary

Different constraints may share the same predicate, in this case, we
will generate duplicate ODS verification function.

[jpienaar]

Nice, could you add a test? Any measurement of how much duplicate functions were being generated?

[Chia-hungDuan]

Brief update, there are 136 duplicates out of 712 verifiers.

[Chia-hungDuan]

Added test

[rriddle]

Thanks for looking into this! Looks like a great improvement!

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

222

Can we use a DenseMap of SmallVector instead?

256

Can we just add a DenseMapInfo for Pred instead of adding the opaque access API?

[jpienaar]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

232

Could you add a comment here too? I know you have above, but one here saying something like "Ensure the same error message is reported" or "Avoid merging verification what would produce different error messages". Although, come to think of it: in that case, if the same predicate would fail, then the failure is in order and we would not be able to trigger the one post (as this is per op), so do we even need to check this if it cannot be observed? Or is it observable?

[Chia-hungDuan]

Addressed review comments

[Chia-hungDuan]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

232

When predicate fails we need to report the summary of the associated Constraint. Thus a predicate fail may happen under different contexts. For example, ConstraintA is applied on OpA, ConstraintB is applied on OpB and they have the same predicate. We need to report summary of ConstraintA if the predicate fails while verifying OpA, otherwise we need to report summary of ConstraintB.

So far I don't see this case, or we can give an error instead?

[jpienaar]

mlir/include/mlir/TableGen/Predicate.h

17

OOC why is APInt needed here?

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

232

You are correct, I was thinking of predicates on same op vs different ones. And they could indeed be different (nothing stopping one predicate from having different high level info associated with it) so needs to be retained even if we don't do so today.

[Chia-hungDuan]

Removed redundant header

mlir/include/mlir/TableGen/Predicate.h

 //===- Predicate.h - Predicate class ----------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // Wrapper around predicates defined in TableGen.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_TABLEGEN_PREDICATE_H_
 #define MLIR_TABLEGEN_PREDICATE_H_
 
 #include "mlir/Support/LLVM.h"
+#include "llvm/ADT/Hashing.h"
 
 #include <string>
 #include <vector>
 
 namespace llvm {
 class Init;
 class ListInit;
 class Record;
public:
 
   // Records are pointer-comparable.
   bool operator==(const Pred &other) const { return def == other.def; }
 
   // Get the location of the predicate.
   ArrayRef<llvm::SMLoc> getLoc() const;
 
 protected:
+  friend llvm::DenseMapInfo<Pred>;
+
   // The TableGen definition of this predicate.
   const llvm::Record *def;
 };
 
 // A logical predicate wrapping a C expression.  This class must closely follow
 // the definition of TableGen class 'CPred'.
 class CPred : public Pred {
 public:

 public:
   StringRef getPrefix() const;
   StringRef getSuffix() const;
 };
 
 } // end namespace tblgen
 } // end namespace mlir
 
+namespace llvm {
+template <>
+struct DenseMapInfo<mlir::tblgen::Pred> {
+  static mlir::tblgen::Pred getEmptyKey() { return mlir::tblgen::Pred(); }
+  static mlir::tblgen::Pred getTombstoneKey() { return mlir::tblgen::Pred(); }
+  static unsigned getHashValue(mlir::tblgen::Pred pred) {
+    return llvm::hash_value(pred.def);
+  }
+  static bool isEqual(mlir::tblgen::Pred lhs, mlir::tblgen::Pred rhs) {
+    return lhs == rhs;
+  }
+};
+} // end namespace llvm
+
 #endif // MLIR_TABLEGEN_PREDICATE_H_

mlir/test/mlir-tblgen/predicate.td

 // RUN: mlir-tblgen -gen-op-defs -I %S/../../include %s | FileCheck %s
 
 include "mlir/IR/OpBase.td"
 
 def Test_Dialect : Dialect {
   let name = "test";
 }
 class NS_Op<string mnemonic, list<OpTrait> traits> :
     Op<Test_Dialect, mnemonic, traits>;
 
 def I32OrF32 : Type<CPred<"$_self.isInteger(32) || $_self.isF32()">,
                     "32-bit integer or floating-point type">;
 
 def OpA : NS_Op<"op_for_CPred_containing_multiple_same_placeholder", []> {
   let arguments = (ins I32OrF32:$x);
+  let results = (outs Variadic<I32OrF32>:$y);
 }
 
 // CHECK: static ::mlir::LogicalResult [[$INTEGER_FLOAT_CONSTRAINT:__mlir_ods_local_type_constraint.*]](
-// CHECK:  if (!((type.isInteger(32) || type.isF32()))) {
-// CHECK:    return op->emitOpError(valueKind) << " #" << valueGroupStartIndex << " must be 32-bit integer or floating-point type, but got " << type;
+// CHECK-NEXT:  if (!((type.isInteger(32) || type.isF32()))) {
+// CHECK-NEXT:    return op->emitOpError(valueKind) << " #" << valueGroupStartIndex << " must be 32-bit integer or floating-point type, but got " << type;
+
+// Check there is no verifier with same predicate generated.
+// CHECK-NOT:  if (!((type.isInteger(32) || type.isF32()))) {
+// CHECK-NOT:    return op->emitOpError(valueKind) << " #" << valueGroupStartIndex << " must be 32-bit integer or floating-point type, but got " << type;
 
 // CHECK: static ::mlir::LogicalResult [[$TENSOR_CONSTRAINT:__mlir_ods_local_type_constraint.*]](
-// CHECK:  if (!(((type.isa<::mlir::TensorType>())) && ((true)))) {
-// CHECK:    return op->emitOpError(valueKind) << " #" << valueGroupStartIndex << " must be tensor of any type values, but got " << type;
+// CHECK-NEXT:  if (!(((type.isa<::mlir::TensorType>())) && ((true)))) {
+// CHECK-NEXT:    return op->emitOpError(valueKind) << " #" << valueGroupStartIndex << " must be tensor of any type values, but got " << type;
 
 // CHECK: static ::mlir::LogicalResult [[$TENSOR_INTEGER_FLOAT_CONSTRAINT:__mlir_ods_local_type_constraint.*]](
-// CHECK:  if (!(((type.isa<::mlir::TensorType>())) && (((type.cast<::mlir::ShapedType>().getElementType().isF32())) || ((type.cast<::mlir::ShapedType>().getElementType().isSignlessInteger(32)))))) {
-// CHECK:    return op->emitOpError(valueKind) << " #" << valueGroupStartIndex << " must be tensor of 32-bit float or 32-bit signless integer values, but got " << type;
+// CHECK-NEXT:  if (!(((type.isa<::mlir::TensorType>())) && (((type.cast<::mlir::ShapedType>().getElementType().isF32())) || ((type.cast<::mlir::ShapedType>().getElementType().isSignlessInteger(32)))))) {
+// CHECK-NEXT:    return op->emitOpError(valueKind) << " #" << valueGroupStartIndex << " must be tensor of 32-bit float or 32-bit signless integer values, but got " << type;
 
 // CHECK-LABEL: OpA::verify
 // CHECK: auto valueGroup0 = getODSOperands(0);
 // CHECK: for (::mlir::Value v : valueGroup0) {
 // CHECK:   if (::mlir::failed([[$INTEGER_FLOAT_CONSTRAINT]]
 
 def OpB : NS_Op<"op_for_And_PredOpTrait", [
     PredOpTrait<"both first and second holds",

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

for (Record *def : opDefs) {
     for (NamedTypeConstraint &operand : op.getOperands())
       if (operand.hasPredicate())
         typeConstraints.insert(operand.constraint.getAsOpaquePointer());
     for (NamedTypeConstraint &result : op.getResults())
       if (result.hasPredicate())
         typeConstraints.insert(result.constraint.getAsOpaquePointer());
   }
 
+  // Record the mapping from predicate to constraint. If two constraints has the
+  // same predicate and constraint summary, they can share the same verification
+  // function.
+  llvm::DenseMap<Pred, const void *> predToConstraint;
   FmtContext fctx;
   for (auto it : llvm::enumerate(typeConstraints)) {
+    std::string name;
+    Constraint constraint = Constraint::getFromOpaquePointer(it.value());
+    Pred pred = constraint.getPredicate();
+    auto iter = predToConstraint.find(pred);
+    if (iter != predToConstraint.end()) {
+      do {
+        Constraint built = Constraint::getFromOpaquePointer(iter->second);
+        // We may have the different constraints but have the same predicate,
+        // for example, ConstraintA and Variadic<ConstraintA>, note that
+        // Variadic<> doesn't introduce new predicate. In this case, we can
+        // share the same predicate function if they also have consistent
+        // summary, otherwise we may report the wrong message while verification
+        // fails.
+        if (constraint.getSummary() == built.getSummary()) {
+          name = getTypeConstraintFn(built).str();
+          break;
+        }
+        ++iter;
+      } while (iter != predToConstraint.end() && iter->first == pred);
+    }
+
+    if (!name.empty()) {
+      localTypeConstraints.try_emplace(it.value(), name);
+      continue;
+    }
+
     // Generate an obscure and unique name for this type constraint.
-    std::string name = (Twine("__mlir_ods_local_type_constraint_") +
-                        uniqueOutputLabel + Twine(it.index()))
-                           .str();
+    name = (Twine("__mlir_ods_local_type_constraint_") + uniqueOutputLabel +
+            Twine(it.index()))
+    predToConstraint.insert(
+        std::make_pair(constraint.getPredicate(), it.value()));
     localTypeConstraints.try_emplace(it.value(), name);
 
     // Only generate the methods if we are generating definitions.
     if (emitDecl)
       continue;
 
-    Constraint constraint = Constraint::getFromOpaquePointer(it.value());
     os << "static ::mlir::LogicalResult " << name
        << "(::mlir::Operation *op, ::mlir::Type type, ::llvm::StringRef "
           "valueKind, unsigned valueGroupStartIndex) {\n";
 
     os << "  if (!("
        << tgfmt(constraint.getConditionTemplate(), &fctx.withSelf("type"))
        << ")) {\n"
        << formatv(