This is an archive of the discontinued LLVM Phabricator instance.

Change the DebugAction paradigm to delegate the control to the handler
ClosedPublic

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

Details

Summary

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

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

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
mlir/include/mlir/Support/DebugAction.h
153–154

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.
mlir/include/mlir/Support/DebugAction.h
106

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.
mlir/include/mlir/Support/DebugAction.h
173–186

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
mlir/include/mlir/Support/DebugAction.h
93

Why the change out of debug-only for DebugAction?

148

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

261

New editor defaults off ?

mehdi_amini marked 6 inline comments as done.

address comments

mlir/include/mlir/Support/DebugAction.h
93

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

148

Sure, but note that this all goes away in D144811

173–186

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.

See https://reviews.llvm.org/D144812

Or the diagram in the RFC: https://discourse.llvm.org/t/rfc-mlir-action-tracing-and-debugging-mlir-based-compilers/68679/6

https://global.discourse-cdn.com/business4/uploads/llvm/original/3X/b/7/b73cd3ef38ba613acfd23305ac47ed4ddb856bbc.jpeg

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.