Page MenuHomePhabricator

[mlir] Add a new debug action framework.
ClosedPublic

Authored by rriddle on Jul 30 2020, 2:50 PM.

Details

Summary

This revision adds the infrastructure for Debug Actions. This is a DEBUG only
API that allows for external entities to control various aspects of compiler
execution. This is conceptually similar to something like DebugCounters in LLVM, but at a lower level. This framework doesn't make any assumptions about how the higher level driver is controlling the execution, it merely provides a framework for connecting the two together. This means that on top of DebugCounter functionality, we could also provide more interesting drivers such as interactive execution. A high level overview of the workflow surrounding debug actions is
shown below:

  • Compiler developer defines an action that is taken by the a pass, transformation, utility that they are developing.
  • Depending on the needs, the developer dispatches various queries, pertaining to this action, to an action manager that will provide an answer as to what behavior the action should do.
  • An external entity registers an action handler with the action manager, and provides the logic to resolve queries on actions.

The exact definition of an external entity is left opaque, to allow for more
interesting handlers.

This framework was proposed here: https://llvm.discourse.group/t/rfc-debug-actions-in-mlir-debug-counters-for-the-modern-world

Diff Detail

Event Timeline

rriddle created this revision.Jul 30 2020, 2:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2020, 2:50 PM
rriddle requested review of this revision.Jul 30 2020, 2:50 PM

Can you clang-format and rebase?

Can you clang-format and rebase?

Done.

jpienaar accepted this revision.Feb 18 2021, 3:20 PM

Overall looks good, thanks. I need to look at the follow up. I'd be very interested to see how we could hook this up with some user input/interactive stepping later :)

mlir/include/mlir/Support/DebugAction.h
133

Is the preferred method and the above the generic one? It would seem like the above could almost be private.

154

This is weird in the context of a failed status returned. Is FailureOr appropriate? E.g., is it really showing "handler registered and it returned true", so that if no handler registered then it runs. Perhaps a different type could help.

I mean, it failed to dispatch to handler as there is no handler, but also it could have been considered a success as there is no handler.

161

Is there a way to ensure all is compiled away outside debug mode? Maybe by having NDEBUG over larger sections/some code duplicated. That would make the NOP nature easier to see (at least for me :))

mlir/unittests/Support/DebugActionTest.cpp
63

This works quite nicely. Could you show one where either it runs up until 5th iteration? And a test for printing out the formatted arg strings?

This revision is now accepted and ready to land.Feb 18 2021, 3:20 PM
mehdi_amini added inline comments.Feb 19 2021, 11:46 AM
mlir/docs/DebugActions.md
16

Something that wasn't clear to me when reading this was how would the developer define the queries. It turns out there is a pre-defined fixed list of queries that are available globally on the action manager for all possible actions.

rriddle updated this revision to Diff 325126.Feb 19 2021, 6:30 PM
rriddle marked 5 inline comments as done.

update

rriddle added inline comments.Feb 19 2021, 6:31 PM
mlir/docs/DebugActions.md
16

I want to relax the static aspect of this, but haven't yet finalized thoughts on how to generically+opaquely add external queries and keep everything nice and type safe. The section on the action manager mentions the set of available queries. Added more links here.

mlir/include/mlir/Support/DebugAction.h
133

This one will probably be used more frequently, but didn't want to prevent the above one if there is additional initialization necessary when creating the handler. For example, this is used in the debug counter in the child revision.

154

Discussed offline, keeping as FailureOr for now as that matches the diagnostic engine behavior. In a followup it may be interesting to add an UnhandledOr alias for FailureOr that better describes the real behavior.

161

Commented out the entire private section, and the internals of the public methods.

mehdi_amini added inline comments.Feb 19 2021, 8:13 PM
mlir/include/mlir/Support/DebugAction.h
215

I'm not sure about the right behavior for dispatchToHandler here.
Seems like the last registered handler will process the query and the others won't get it, does it generalize to every kind of query?
Wouldn't we want something that could log and let other handler manages the query as well?

I guess for a query that returns a value you need to figure what value to return, so the first handler that gets you a value may be reasonable. For void queries or any kind of tracing, it may not be the same.

254

What is the purpose of accepting this? Do you have examples of how it works?

In general the whole "arguments as string" is a bit weird to me.

rriddle updated this revision to Diff 325638.Feb 22 2021, 6:40 PM
rriddle marked 2 inline comments as done.

Update

rriddle added inline comments.Feb 22 2021, 6:42 PM
mlir/include/mlir/Support/DebugAction.h
215

I'm not sure yet either TBH. Each query would likely have it's own kind of reduction behavior, but picking the first one seems fine for me until we have a use case that really needs/wants something smarter.

254

Just removed opaque parameters for now.

mehdi_amini added inline comments.Feb 22 2021, 7:16 PM
mlir/include/mlir/Support/DebugAction.h
215

Right, maybe you can make mention of this in the doc?

rriddle marked an inline comment as done.Feb 22 2021, 7:30 PM
mehdi_amini accepted this revision.Feb 22 2021, 8:19 PM
This revision was landed with ongoing or failed builds.Feb 23 2021, 1:02 AM
This revision was automatically updated to reflect the committed changes.