Page MenuHomePhabricator

[mlir] Support collecting logs from notifyMatchFailure().
ClosedPublic

Authored by Chia-hungDuan on Sep 30 2021, 5:26 PM.

Details

Summary

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

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Sep 30 2021, 5:26 PM
Chia-hungDuan requested review of this revision.Sep 30 2021, 5:26 PM

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.

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.

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.

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.

Code simplify

rriddle requested changes to this revision.Oct 4 2021, 4:04 PM

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.

This revision now requires changes to proceed.Oct 4 2021, 4:04 PM

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?

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.

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

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.

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.

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 updated this revision to Diff 387807.EditedTue, Nov 16, 6:11 PM

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 added inline comments.Wed, Nov 17, 9:39 PM
mlir/include/mlir/Transforms/DialectConversion.h
529–530

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).

939

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
1478

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 marked an inline comment as done.

Address review comment

mlir/include/mlir/Transforms/DialectConversion.h
939

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
1478

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

rriddle added inline comments.Fri, Dec 3, 1:51 PM
mlir/include/mlir/Transforms/DialectConversion.h
935–949

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

936–938
mlir/lib/Transforms/Utils/DialectConversion.cpp
1488–1499

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

1960–1965

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

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
935–949

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

mlir/lib/Transforms/Utils/DialectConversion.cpp
1960–1965

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} : () -> ()

Address review comment

rriddle accepted this revision.Fri, Dec 3, 7:32 PM
This revision is now accepted and ready to land.Fri, Dec 3, 7:32 PM