Let the user registers their own handler to processing the matching
failure information.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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.
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.
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.
mlir/include/mlir/Transforms/DialectConversion.h | ||
---|---|---|
501–502 | 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). | |
911 | 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 | ||
1487 | 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? |
Address review comment
mlir/include/mlir/Transforms/DialectConversion.h | ||
---|---|---|
911 | 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 .... | |
mlir/lib/Transforms/Utils/DialectConversion.cpp | ||
1487 | Reverted back to use LLVM_DEBUG I think that's enough for the initial support |
mlir/include/mlir/Transforms/DialectConversion.h | ||
---|---|---|
907–910 | Is this only intended for analysis conversions, or should we do this for partial/full conversions too? | |
908–910 | ||
mlir/lib/Transforms/Utils/DialectConversion.cpp | ||
1497–1508 | nit: I would just inline all of these into the class. | |
1983–1988 | 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 | ||
---|---|---|
907–910 | Per discussion, now we will only have it in analysis conversions. | |
mlir/lib/Transforms/Utils/DialectConversion.cpp | ||
1983–1988 | 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} : () -> () |
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).