[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

223

Can we use a DenseMap of SmallVector instead?

252

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

[jpienaar]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

233

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

233

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

233

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 <string>
 #include <vector>
 
 namespace llvm {
 class Init;
 class ListInit;
 class Record;
 class SMLoc;
public:
   bool isCombined() const;
 
   // 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;
 
+  // Get an opaque pointer to this predicate.
+  const void *getAsOpaquePointer() const { return def; }
+  // Construct a predicate from the opaque pointer representation.
+  static Pred getFromOpaquePointer(const void *ptr) {
+    return Pred(reinterpret_cast<const llvm::Record *>(ptr));
+  }
+
 protected:
   // 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 {

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

 #include "llvm/ADT/Sequence.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/TableGen/Error.h"
 #include "llvm/TableGen/Record.h"
 #include "llvm/TableGen/TableGenBackend.h"
 
+#include <unordered_map>
+
 #define DEBUG_TYPE "mlir-tblgen-opdefgen"
 
 using namespace llvm;
 using namespace mlir;
 using namespace mlir::tblgen;
 
 static const char *const tblgenNamePrefix = "tblgen_";
 static const char *const generatedArgName = "odsArg";
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.
+  std::unordered_multimap<const void *, 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.getAsOpaquePointer());
+    if (iter != predToConstraint.end()) {
+      do {
+        Constraint built = Constraint::getFromOpaquePointer(iter->second);
+        if (constraint.getSummary() == built.getSummary()) {
+          name = getTypeConstraintFn(built).str();
+          break;
+        }
+        ++iter;
+      } while (iter != predToConstraint.end() &&
+               iter->first == pred.getAsOpaquePointer());
+    }
+
+    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().getAsOpaquePointer(), 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(