[mlir] Debug print pattern before and after matchAndRewrite call

[Hardcode84]

Summary

Motivation: we have passes with lot of rewrites and when one one them segfaults or asserts, it is very hard to find waht exactly pattern failed without debug info.

[rriddle]

mlir/include/mlir/IR/PatternMatch.h

196

I would consider the name as part of the pattern, i.e. the name should be set in the constructor or via a set method by the pattern itself. There shouldn't be a need to expose this to any other class.

[Hardcode84]

mlir/include/mlir/IR/PatternMatch.h

196

This makes sense from architectural POV but I'm not sure how to achieve this. And I definitely do not want to bother pattern writers with additional ctor parameter, needed only for debug purposes.

[rriddle]

mlir/include/mlir/IR/PatternMatch.h

196

I have the opposite POV. I'd rather do what's right architecturally. I don't have as much concern about pattern writers adding an additional param.

[Hardcode84]

mlir/include/mlir/IR/PatternMatch.h

196

This is work which can and should be automated. Not to mention it will require update to all existing rewrites to be useful, which is unrealistic. Anyway, I agree that current solution is hacky and I don't have better at the moment. I guess I will just keep this as separate patch and will apply to my copy locally.

[rriddle]

mlir/include/mlir/IR/PatternMatch.h

196

I don't think it's completely hacky, and I do have a need/desire for this functionality as well. What if we restructured it such that:

• A pattern may have multiple labels.
• A public function on Pattern is exposed, e.g. addLabel(StringRef label)

At that point, you can move the need for any friends (which was the part that stuck out to me most).

[mehdi_amini]

I feel this is a bit "ad-hoc" and without more info here it may be a bit messy (collision, lack of naming convention, etc.) . It may not matter too much as a "debug name" use case but that could prevent other kind of use cases (filtering for example). A label system could be more flexible but "label" does not necessarily provide a unique id either. I'm not sure what'd be an ideal solution yet though.

[Hardcode84]

Actually, maybe just rename those methods to get/setDebugName() or setPrettyName(), make them public and say explicitly in doc that it should be used only for debugging purposes? So 'proper' pattern name or tags can be added later separately if needed.

[rriddle]

In D101443#2727006, @Hardcode84 wrote:

Actually, maybe just rename those methods to get/setDebugName() or setPrettyName(), make them public and say explicitly in doc that it should be used only for debugging purposes? So 'proper' pattern name or tags can be added later separately if needed.

SGTM. I can evolve it to fit my needs in a followup.

mlir/lib/Rewrite/PatternApplicator.cpp

202

Do we need to repeat the Trying to match here? Seems redundant?

[Hardcode84]

Renamed to setDebugName, moved to public, FnPattern handling

mlir/lib/Rewrite/PatternApplicator.cpp

202

removed

[rriddle]

mlir/include/mlir/IR/PatternMatch.h

994–1002

?

mlir/lib/Rewrite/PatternApplicator.cpp

199–200

Is this useful? Is the result much worse if we just emit the empty name?

(Note that getPatternName is also going to need a (void)getPatternName to avoid unused variable errors in release mode)

[Hardcode84]

mlir/lib/Rewrite/PatternApplicator.cpp

199–200

This is debug output, so I don't have strong opinion on this. Empty pattern name will look like Trying to match "" (empty quotes)

[Hardcode84]

review comments

[rriddle]

Do you need someone to land this for you?

mlir/lib/Rewrite/PatternApplicator.cpp

199–200

If it matters in-practice we can look at adding better defaults.

mlir/include/mlir/IR/PatternMatch.h

 //===- PatternMatch.h - PatternMatcher classes -------==---------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_PATTERNMATCHER_H
 #define MLIR_PATTERNMATCHER_H
 
 #include "mlir/IR/Builders.h"
 #include "mlir/IR/BuiltinOps.h"
 #include "llvm/ADT/FunctionExtras.h"
+#include "llvm/Support/TypeName.h"
 
 namespace mlir {
 
 class PatternRewriter;
 
 //===----------------------------------------------------------------------===//
 // PatternBenefit class
 //===----------------------------------------------------------------------===//
bool hasBoundedRewriteRecursion() const {
     return contextAndHasBoundedRecursion.getInt();
   }
 
   /// Return the MLIRContext used to create this pattern.
   MLIRContext *getContext() const {
     return contextAndHasBoundedRecursion.getPointer();
   }
 
+  /// Return readable pattern name. Should only be used for debugging purposes.
+  /// Can be empty.
+  StringRef getDebugName() const { return debugName; }
+
+  /// Set readable pattern name. Should only be used for debugging purposes.
+  void setDebugName(StringRef name) { debugName = name; }
+
 protected:
   /// This class acts as a special tag that makes the desire to match "any"
   /// operation type explicit. This helps to avoid unnecessary usages of this
   /// feature, and ensures that the user is making a conscious decision.
   struct MatchAnyOpTypeTag {};
   /// This class acts as a special tag that makes the desire to match any
   /// operation that implements a given interface explicit. This helps to avoid
   /// unnecessary usages of this feature, and ensures that the user is making a
protected:
   /// Set the flag detailing if this pattern has bounded rewrite recursion or
   /// not.
   void setHasBoundedRewriteRecursion(bool hasBoundedRecursionArg = true) {
     contextAndHasBoundedRecursion.setInt(hasBoundedRecursionArg);
   }
 
 private:
   Pattern(const void *rootValue, RootKind rootKind,
           ArrayRef<StringRef> generatedNames, PatternBenefit benefit,
           MLIRContext *context);
 
   /// The value used to match the root operation of the pattern.
   const void *rootValue;
   RootKind rootKind;
 
   /// The expected benefit of matching this pattern.
   const PatternBenefit benefit;
 
   /// The context this pattern was created from, and a boolean flag indicating
   /// whether this pattern has bounded recursion or not.
   llvm::PointerIntPair<MLIRContext *, 1, bool> contextAndHasBoundedRecursion;
 
   /// A list of the potential operations that may be generated when rewriting
   /// an op with this pattern.
   SmallVector<OperationName, 2> generatedOps;
+
+  /// Readable pattern name. Can be empty.
+  StringRef debugName;
 };
 
 //===----------------------------------------------------------------------===//
 // RewritePattern
 //===----------------------------------------------------------------------===//
 
 /// RewritePattern is the common base class for all DAG to DAG replacements.
 /// There are two possible usages of this class:
public:
 
   // Add a matchAndRewrite style pattern represented as a C function pointer.
   template <typename OpType>
   RewritePatternSet &
   insert(LogicalResult (*implFn)(OpType, PatternRewriter &rewriter)) {
     struct FnPattern final : public OpRewritePattern<OpType> {
       FnPattern(LogicalResult (*implFn)(OpType, PatternRewriter &rewriter),
                 MLIRContext *context)
-          : OpRewritePattern<OpType>(context), implFn(implFn) {}
+          : OpRewritePattern<OpType>(context), implFn(implFn) {
+        setDebugName(llvm::getTypeName<FnPattern>());
+      }
 
       LogicalResult matchAndRewrite(OpType op,
                                     PatternRewriter &rewriter) const override {
         return implFn(op, rewriter);
       }
 
     private:
       LogicalResult (*implFn)(OpType, PatternRewriter &rewriter);
     };
     insert(std::make_unique<FnPattern>(std::move(implFn), getContext()));
     return *this;
   }
 
 private:
   /// Add an instance of the pattern type 'T'. Return a reference to `this` for
   /// chaining insertions.
   template <typename T, typename... Args>
   std::enable_if_t<std::is_base_of<RewritePattern, T>::value>
   addImpl(Args &&... args) {
-    nativePatterns.emplace_back(
-        std::make_unique<T>(std::forward<Args>(args)...));
+    auto pattern = std::make_unique<T>(std::forward<Args>(args)...);
+
+    // Pattern can potentially set name in ctor. Preserve old name if present.
+    if (pattern->getDebugName().empty())
+      pattern->setDebugName(llvm::getTypeName<T>());
+
+    nativePatterns.emplace_back(std::move(pattern));
   }
   template <typename T, typename... Args>
   std::enable_if_t<std::is_base_of<PDLPatternModule, T>::value>
   addImpl(Args &&... args) {
     pdlPatterns.mergeIn(T(std::forward<Args>(args)...));
   }
 
   MLIRContext *const context;
   NativePatternListT nativePatterns;
   PDLPatternModule pdlPatterns;
 };
 
 } // end namespace mlir
 
 #endif // MLIR_PATTERN_MATCH_H

mlir/lib/Rewrite/PatternApplicator.cpp

#ifndef NDEBUG
     // Operation `op` may be invalidated after applying the rewrite pattern.
     Operation *dumpRootOp = getDumpRootOp(op);
 #endif
     if (pdlMatch) {
       bytecode->rewrite(rewriter, *pdlMatch, *mutableByteCodeState);
       result = success(!onSuccess || succeeded(onSuccess(*bestPattern)));
     } else {
       const auto *pattern = static_cast<const RewritePattern *>(bestPattern);
+
+      LLVM_DEBUG(llvm::dbgs()
+                 << "Trying to match \"" << pattern->getDebugName() << "\"\n");
       result = pattern->matchAndRewrite(op, rewriter);
+      LLVM_DEBUG(llvm::dbgs() << "\"" << pattern->getDebugName() << "\" result "
+                              << succeeded(result) << "\n");
+
       if (succeeded(result) && onSuccess && failed(onSuccess(*pattern)))
         result = failure();
     }
     if (succeeded(result)) {
       LLVM_DEBUG(logSucessfulPatternApplication(dumpRootOp));
       break;
     }