This is an archive of the discontinued LLVM Phabricator instance.

Change the DebugAction paradigm to delegate the control to the handler

Authored by mehdi_amini on Feb 25 2023, 10:18 PM.



At the moment, we invoke shouldExecute() that way:

if (manager.shouldExecute<DebugAction>(currentOp) {
  // apply a transformation

In this sequence, the manager isn’t involved in the actual execution
of the action and can’t develop rich instrumentations. Instead the API
could let the control to the handler itself:

// Execute the action under the control of the manager
manager.execute<DebugAction>(currentOp, [&]() {
  // apply the transformation in this callback

This inversion of control (by injecting a callback) allows handlers to
implement potentially new interesting features: for example, snapshot
the IR before and after the action, or record an action execution time.
More importantly, it will allow to capture the nesting execution of

On the other side: handlers receives now a DebugAction object that wraps
generic information (tag and description especially) as well as
action-specific data.

Finally, the DebugActionManager is now enabled in release builds as

Diff Detail

Event Timeline

mehdi_amini created this revision.Feb 25 2023, 10:18 PM
mehdi_amini requested review of this revision.Feb 25 2023, 10:18 PM
zero9178 added inline comments.Feb 26 2023, 2:58 AM

You forgot to remove this #endif here

Fix the include guard

rriddle accepted this revision.Feb 27 2023, 9:32 AM
rriddle added inline comments.

Can we drop the llvm:: on all of these?

This revision is now accepted and ready to land.Feb 27 2023, 9:32 AM
scotttodd accepted this revision.Feb 27 2023, 3:42 PM
scotttodd added inline comments.

This had a comment before:

// TODO: We currently just pick the first handler that gives us a result,
// but in the future we may want to employ a reduction over all of the
// values returned.

Would that still be useful? It seems like being able to register multiple read-only listeners/observers would be useful, but this looks like only one can run at a time?

jpienaar added inline comments.Feb 27 2023, 9:03 PM

Why the change out of debug-only for DebugAction?


Could you add a comment here? (Failed feels weird for me here.)


New editor defaults off ?

mehdi_amini marked 6 inline comments as done.

address comments


It's in the name: this isn't "DebugAction" anymore, but Action :)

(it's broken out in various patches, but look at the follow up in D144809


Sure, but note that this all goes away in D144811


You won't be able to do it on the MLIRContext, this will be exposed by the ExecutionContext though. We couldn't compose things properly at this level here.


Or the diagram in the RFC:

This revision was landed with ongoing or failed builds.Mar 6 2023, 6:59 AM
This revision was automatically updated to reflect the committed changes.