[mlir] Support collecting logs from notifyMatchFailure().

[Chia-hungDuan]

Summary

Let the user registers their own handler to processing the matching
failure information.

[jpienaar]

Why isn't all the methods guarded behind LLVM_DEBUG (or similar debug macro/define)? Seems like that would remove chance of unexpected behavior and make it explicitly for debugging. Downside is then folks can't enable debugging without using the same guarding - but I think that makes expectations clearer.

[mehdi_amini]

In D110896#3036503, @jpienaar wrote:

Why isn't all the methods guarded behind LLVM_DEBUG (or similar debug macro/define)? Seems like that would remove chance of unexpected behavior and make it explicitly for debugging. Downside is then folks can't enable debugging without using the same guarding - but I think that makes expectations clearer.

In headers, it would have to be #if LLVM_ENABLE_ABI_BREAKING_CHECKS or something like that.

[Chia-hungDuan]

In D110896#3036503, @jpienaar wrote:

Why isn't all the methods guarded behind LLVM_DEBUG (or similar debug macro/define)? Seems like that would remove chance of unexpected behavior and make it explicitly for debugging. Downside is then folks can't enable debugging without using the same guarding - but I think that makes expectations clearer.

In D110896#3036917, @mehdi_amini wrote:

In D110896#3036503, @jpienaar wrote:

Why isn't all the methods guarded behind LLVM_DEBUG (or similar debug macro/define)? Seems like that would remove chance of unexpected behavior and make it explicitly for debugging. Downside is then folks can't enable debugging without using the same guarding - but I think that makes expectations clearer.

In headers, it would have to be #if LLVM_ENABLE_ABI_BREAKING_CHECKS or something like that.

Let me try if I can turn these all into debug build only gracefully.

[Chia-hungDuan]

Code simplify

[rriddle]

Can you provide more detail on what problem you are trying to solve here? and how this diagnostics support is supposed to compose/be used in then general infra? The current description and the commit itself leads me to believe this is trying to build a general solution for a dialect conversion specific problem. If the problem is just for dialect conversion, there are likely more local solutions that would work as well.

[Chia-hungDuan]

In D110896#3041236, @rriddle wrote:

Can you provide more detail on what problem you are trying to solve here? and how this diagnostics support is supposed to compose/be used in then general infra? The current description and the commit itself leads me to believe this is trying to build a general solution for a dialect conversion specific problem. If the problem is just for dialect conversion, there are likely more local solutions that would work as well.

The user wants to create a report for a failed dialect conversion. They would like to collect the failure reason generated during dialect conversion (match failure is the main one) then they can have a report which tells the reasons in detail that why an operation can't be converted. In the usage, they will have a debug build tool to run over those failed cases again so that they will have debugging message there. And they would like to have a solution that they can capture the message while conversion so that they don't need to parse/classify the logs.

I think this approach is lightweight and yes, it's a general solution which I think it avoids adding some hooks like passing callback for certain path. What do you think?

[Chia-hungDuan]

In D110896#3041262, @Chia-hungDuan wrote:

In D110896#3041236, @rriddle wrote:

Can you provide more detail on what problem you are trying to solve here? and how this diagnostics support is supposed to compose/be used in then general infra? The current description and the commit itself leads me to believe this is trying to build a general solution for a dialect conversion specific problem. If the problem is just for dialect conversion, there are likely more local solutions that would work as well.

The user wants to create a report for a failed dialect conversion. They would like to collect the failure reason generated during dialect conversion (match failure is the main one) then they can have a report which tells the reasons in detail that why an operation can't be converted. In the usage, they will have a debug build tool to run over those failed cases again so that they will have debugging message there. And they would like to have a solution that they can capture the message while conversion so that they don't need to parse/classify the logs.

I think this approach is lightweight and yes, it's a general solution which I think it avoids adding some hooks like passing callback for certain path. What do you think?

One thing to add, user can achieve this by using ScopedDiagnosticHandler as well but I'm thinking if we can provide one like this then they don't need to worry about how to keep the original diagnostic handler work as usual.

[Chia-hungDuan]

Add a way to group the diagnostics. It means we can group the related
diagnostics together to produce better debugging message.

[rriddle]

How do you see all of this being used outside of the dialect conversion use case? The point I made earlier was whether we should start with something that is passed directly to dialect conversion instead of changing the general diagnostics API. If this is something that is only serving a specific use case, we should keep the scope of it limited.

[Chia-hungDuan]

In D110896#3105051, @rriddle wrote:

How do you see all of this being used outside of the dialect conversion use case? The point I made earlier was whether we should start with something that is passed directly to dialect conversion instead of changing the general diagnostics API. If this is something that is only serving a specific use case, we should keep the scope of it limited.

I thought last time I mentioned that passing the callback in dialect conversion path it seems to be hacky and you mentioned that accessing the diagnostic may be useful(sorry, correct me if I misunderstand). Then we talked if we want to let the user use this, it'll be better to make this more useful (last change only gave a handler wrapper), like if we can let them know the set of diagnostics which are associated with a match failure of certain operation.

So far, the case is only focusing on collecting the message output during pattern matching. I'm not sure if they would like to collect more information other than dialect conversion. Thus I tried to make this for general path and keep as little as impact in normal path.

If this is not a right way, I will suggest the user to report the diagnostics(not through rewriter.notifyMatchFailure()) in their patterns and register the handler to read them. Except the patterns written in DRR, others can be handled well.

[rriddle]

In D110896#3105057, @Chia-hungDuan wrote:

In D110896#3105051, @rriddle wrote:

How do you see all of this being used outside of the dialect conversion use case? The point I made earlier was whether we should start with something that is passed directly to dialect conversion instead of changing the general diagnostics API. If this is something that is only serving a specific use case, we should keep the scope of it limited.

I thought last time I mentioned that passing the callback in dialect conversion path it seems to be hacky and you mentioned that accessing the diagnostic may be useful(sorry, correct me if I misunderstand). Then we talked if we want to let the user use this, it'll be better to make this more useful (last change only gave a handler wrapper), like if we can let them know the set of diagnostics which are associated with a match failure of certain operation.

So far, the case is only focusing on collecting the message output during pattern matching. I'm not sure if they would like to collect more information other than dialect conversion. Thus I tried to make this for general path and keep as little as impact in normal path.

If this is not a right way, I will suggest the user to report the diagnostics(not through rewriter.notifyMatchFailure()) in their patterns and register the handler to read them. Except the patterns written in DRR, others can be handled well.

I see, seems like some wires got crossed. Let's sync this week to discuss it.

[Chia-hungDuan]

Per our discussion, add a specific way to support collecting the matching failure message for now
Only add it in applyAnalysisConversion because the user will only use it on that.

[rriddle]

mlir/include/mlir/Transforms/DialectConversion.h

501–502 ↗

(On Diff #387807)

Cab we initialize it in the impl separately? Having it here leaks the implementation details a bit. (This constructor really shouldn't be public anyways).

914 ↗

(On Diff #387807)

Can you document the callback here? What invariants is it supposed to provide? How would a potential user interact with it/know when they would want to provide it? Also, do we only want the diagnostic here? What about the pattern/op being matched/etc.? What information is useful to provide back?

mlir/lib/Transforms/Utils/DialectConversion.cpp

1297

This isn't the same as LLVM_DEBUG. LLVM_DEBUG also handles checking if the current debug mode enabled is dialect conversion. Can we just have a bool that is defaulted to true/false depending on notifyCallback, that gets explicitly set to true inside of an LLVM_DEBUG code block?

[Chia-hungDuan]

Address review comment

mlir/include/mlir/Transforms/DialectConversion.h

914 ↗

(On Diff #387807)

Also added the log of the pattern and operation. According to the current architecture, The match failure reason will come before pattern name, it'll be like,

match failure ....
Failed to apply pattern 'Foo' on BarOp

mlir/lib/Transforms/Utils/DialectConversion.cpp

1297

Reverted back to use LLVM_DEBUG I think that's enough for the initial support

[rriddle]

mlir/include/mlir/Transforms/DialectConversion.h

907–910 ↗

(On Diff #388672)

Is this only intended for analysis conversions, or should we do this for partial/full conversions too?

908–910 ↗

(On Diff #388672)

mlir/lib/Transforms/Utils/DialectConversion.cpp

1307–1318

nit: I would just inline all of these into the class.

1798–1803

What does the pattern debug log output look like with this?

[Chia-hungDuan]

The messages captured will be looks like

// The failure reason of applying a certain pattern, pattern name comes after this
expected single f32 operand                                                         
                                                                                    
// The pattern name and the operation it tried to apply
Failed to apply pattern "(anonymous namespace)::TestChangeProducerTypeF32ToF64" on op: 
%0 = "test.type_producer"() : () -> bf16

mlir/include/mlir/Transforms/DialectConversion.h

907–910 ↗

(On Diff #388672)

Per discussion, now we will only have it in analysis conversions.

mlir/lib/Transforms/Utils/DialectConversion.cpp

1798–1803

As you mentioned, the curly brackets may have little problem. Added a new line for output. Now it looks like

Failed to apply pattern "(anonymous namespace)::TestRegionRewriteBlockMovement" on op:
"test.region"() ( {
^bb0(%arg0: i64):  // no predecessors
  br ^bb1(%arg0 : i64)
^bb1(%0: i64):  // pred: ^bb0
  "test.invalid"(%0) : (i64) -> ()
}) {legalizer.erase_old_blocks, legalizer.should_clone} : () -> ()

[Chia-hungDuan]

Address review comment

mlir/include/mlir/IR/Diagnostics.h

private:
   /// The unique id for the scoped handler.
   DiagnosticEngine::HandlerID handlerID;
 
   /// The context to erase the handler from.
   MLIRContext *ctx;
 };
 
 //===----------------------------------------------------------------------===//
+// ScopedDiagInspectorHandler
+//===----------------------------------------------------------------------===//
+
+/// This is a RAII class for inspecting the diagnostic. It can be used to
+/// collect the information at a certain program scope. It's mainly for
+/// debugging purpose.
+class ScopedDiagInspectorHandler {
+public:
+  /// An inspector is only supposed to read the diagnostic so it shouldn't
+  /// update it.
+  using InspectorTy = std::function<LogicalResult(const Diagnostic &)>;
+
+  ScopedDiagInspectorHandler(MLIRContext *ctx, InspectorTy inspector)
+      : ctx(ctx), inspector(inspector) {
+    auto &diagEngine = ctx->getDiagEngine();
+    handlerID =
+        diagEngine.registerHandler([this](Diagnostic &diag) -> LogicalResult {
+          (void)this->inspector(diag);
+          // Inspector is supposed to only read the diagnostic, return failure
+          // to let other handlers do their job.
+          return failure();
+        });
+  }
+  ~ScopedDiagInspectorHandler();
+
+private:
+  /// The context to erase the handler from.
+  MLIRContext *ctx;
+
+  /// The inspector registered by the user.
+  InspectorTy inspector;
+
+  /// The handler id returned by the DiagEngine.
+  DiagnosticEngine::HandlerID handlerID;
+};
+
+//===----------------------------------------------------------------------===//
 // SourceMgrDiagnosticHandler
 //===----------------------------------------------------------------------===//
 
 namespace detail {
 struct SourceMgrDiagnosticHandlerImpl;
 } // end namespace detail
 
 /// This class is a utility diagnostic handler for use with llvm::SourceMgr.

mlir/lib/IR/Diagnostics.cpp

 //===----------------------------------------------------------------------===//
 
 ScopedDiagnosticHandler::~ScopedDiagnosticHandler() {
   if (handlerID)
     ctx->getDiagEngine().eraseHandler(handlerID);
 }
 
 //===----------------------------------------------------------------------===//
+// ScopedDiagInspectorHandler
+//===----------------------------------------------------------------------===//
+ScopedDiagInspectorHandler::~ScopedDiagInspectorHandler() {
+  ctx->getDiagEngine().eraseHandler(handlerID);
+}
+
+//===----------------------------------------------------------------------===//
 // SourceMgrDiagnosticHandler
 //===----------------------------------------------------------------------===//
 namespace mlir {
 namespace detail {
 struct SourceMgrDiagnosticHandlerImpl {
   /// Return the SrcManager buffer id for the specified file, or zero if none
   /// can be found.
   unsigned getSourceMgrBufferIDForFile(llvm::SourceMgr &mgr,

mlir/lib/Transforms/Utils/DialectConversion.cpp

void ConversionPatternRewriterImpl::notifyRegionWasClonedBefore(
   // This original region has already had its conversion set computed, so there
   // shouldn't be any new failures.
   (void)result;
   assert(succeeded(result) && "expected region to have no unreachable blocks");
 }
 
 LogicalResult ConversionPatternRewriterImpl::notifyMatchFailure(
     Location loc, function_ref<void(Diagnostic &)> reasonCallback) {
   LLVM_DEBUG({
     Diagnostic diag(loc, DiagnosticSeverity::Remark);
     reasonCallback(diag);
     logger.startLine() << "** Failure : " << diag.str() << "\n";
+    // Emit to the DiagEngine so that the inspector can get notified.
+    loc->getContext()->getDiagEngine().emit(std::move(diag));
   });
   return failure();
 }
 
 //===----------------------------------------------------------------------===//
 // ConversionPatternRewriter
 //===----------------------------------------------------------------------===//
 
 ConversionPatternRewriter::ConversionPatternRewriter(MLIRContext *ctx)
     : PatternRewriter(ctx),
       impl(new detail::ConversionPatternRewriterImpl(*this)) {}
 ConversionPatternRewriter::~ConversionPatternRewriter() {}
 
 /// PatternRewriter hook for replacing the results of an operation when the
 /// given functor returns true.
 void ConversionPatternRewriter::replaceOpWithIf(
     Operation *op, ValueRange newValues, bool *allUsesReplaced,
     llvm::unique_function<bool(OpOperand &) const> functor) {
   // TODO: To support this we will need to rework a bit of how replacements are
   // tracked, given that this isn't guranteed to replace all of the uses of an
   // operation. The main change is that now an operation can be replaced
   // multiple times, in parts. The current "set" based tracking is mainly useful
   // for tracking if a replaced operation should be ignored, i.e. if all of the
   // uses will be replaced.
auto canApply = [&](const Pattern &pattern) {
     return canApplyPattern(op, pattern, rewriter);
   };
 
   // Functor that cleans up the rewriter state after a pattern failed to match.
   RewriterState curState = rewriterImpl.getCurrentState();
   auto onFailure = [&](const Pattern &pattern) {
     LLVM_DEBUG(logFailure(rewriterImpl.logger, "pattern failed to match"));
     rewriterImpl.resetState(curState);
     appliedPatterns.erase(&pattern);
   };
 
   // Functor that performs additional legalization when a pattern is
   // successfully applied.
   auto onSuccess = [&](const Pattern &pattern) {
     auto result = legalizePatternResult(op, pattern, rewriter, curState);
     appliedPatterns.erase(&pattern);
     if (failed(result))
       rewriterImpl.resetState(curState);
     return result;
   };
 
   // Try to match and rewrite a pattern on this operation.

mlir/test/IR/diagnostic-inspector.mlir

+// RUN: mlir-opt -test-diagnostic-inspector %s
+// This test verifies that diagnostic inspector can capture the diagnostics.
+
+func @test_diagnostic_inspector() {
+  "test.dummy_0"() : () -> ()
+  "test.dummy_1"() : () -> ()
+  return
+}

mlir/test/lib/IR/TestDiagnostics.cpp

 //===- TestDiagnostics.cpp - Test Diagnostic Utilities --------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains test passes for constructing and resolving dominance
 // information.
 //
 //===----------------------------------------------------------------------===//
 
+#include "mlir/IR/Diagnostics.h"
 #include "mlir/Pass/Pass.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/SourceMgr.h"
 
+#include <string>
+
 using namespace mlir;
 
 namespace {
+struct TestDiagnosticInspectorPass
+    : public PassWrapper<TestDiagnosticInspectorPass, OperationPass<FuncOp>> {
+  StringRef getArgument() const final { return "test-diagnostic-inspector"; }
+  StringRef getDescription() const final {
+    return "Test diagnostic inspector support.";
+  }
+
+  TestDiagnosticInspectorPass() {}
+  TestDiagnosticInspectorPass(const TestDiagnosticInspectorPass &) {}
+
+  void runOnOperation() override {
+    llvm::SmallVector<std::string> opNames;
+    ScopedDiagInspectorHandler inspector(
+        &getContext(), [&](const Diagnostic &diag) -> LogicalResult {
+          opNames.push_back(diag.str());
+          return success();
+        });
+
+    getOperation()->walk([&](Operation *op) {
+      // Emit the operation name for inspector.
+      op->emitRemark() << op->getName().getStringRef();
+    });
+
+    // Verify the operation names are captured by the inspector.
+    auto nameIter = opNames.begin();
+    getOperation()->walk([&](Operation *op) {
+      assert(op->getName().getStringRef().equals(*nameIter));
+      ++nameIter;
+    });
+  }
+};
+
 struct TestDiagnosticFilterPass
     : public PassWrapper<TestDiagnosticFilterPass, OperationPass<FuncOp>> {
   StringRef getArgument() const final { return "test-diagnostic-filter"; }
   StringRef getDescription() const final {
     return "Test diagnostic filtering support.";
   }
   TestDiagnosticFilterPass() {}
   TestDiagnosticFilterPass(const TestDiagnosticFilterPass &) {}

 };
 
 } // end anonymous namespace
 
 namespace mlir {
 namespace test {
 void registerTestDiagnosticsPass() {
   PassRegistration<TestDiagnosticFilterPass>{};
+  PassRegistration<TestDiagnosticInspectorPass>{};
 }
 } // namespace test
 } // namespace mlir