This is an archive of the discontinued LLVM Phabricator instance.

Introduce mlir::tracing::ExecutionContext
ClosedPublic

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

Details

Summary

This component acts as an action handler that can be registered in the
MLIRContext. It is the main orchestration of the infrastructure, and implements
support for clients to hook there and snoop on or control the execution.
This is the basis to build tracing as well as a "gdb-like" control of the
compilation flow.

The ExecutionContext acts as a handler in the MLIRContext for executing an
Action. When an action is dispatched, it'll query its set of Breakpoints
managers for a breakpoint matching this action. If a breakpoint is hit, it
passes the action and the breakpoint information to a callback. The callback
is responsible for controlling the execution of the action through an enum
value it returns. Optionally, observers can be registered to be notified
before and after the callback is executed.

Diff Detail

Event Timeline

mehdi_amini created this revision.Feb 25 2023, 10:19 PM
mehdi_amini requested review of this revision.Feb 25 2023, 10:19 PM
zero9178 added inline comments.Feb 26 2023, 3:27 AM
mlir/include/mlir/Debug/BreakpointManager.h
19

Would it be worth also having a CRTP BreakPointBase class to avoid derived classes having to manully pass TypeID and implement classof?

68

nit: I think having Base as suffix for these kind of base classes is more conventional within MLIR than Impl

mlir/include/mlir/Debug/ExecutionContext.h
30

Should this be unsigned since it can never be negative? I think it'd also be worth documenting these getters, for things like depth returning 0 for the top level, and getParent() returning a nullptr in this case

96

Looks like an incomplete sentence.

mehdi_amini marked 3 inline comments as done.

address comments

mlir/include/mlir/Debug/ExecutionContext.h
30
zero9178 added inline comments.Feb 26 2023, 6:19 AM
mlir/include/mlir/Debug/ExecutionContext.h
30

I see, thanks for the reference!

(started a scan but realized I wasn't really reading with any detail ...)

mlir/include/mlir/Debug/BreakpointManagers/TagBreakpointManager.h
10

TODO ? :)

mlir/include/mlir/Debug/ExecutionContext.h
39

Weird line break here.

106

This seems quite verbose way of saying "Next point to stop execution as describe by Control enum".

108

Missing closing `

109

nextStoppingPoint ? (depth just doesn't match the description above)

120

\n

Tyker added a subscriber: Tyker.Mar 1 2023, 6:17 PM
Tyker added inline comments.
mlir/include/mlir/Debug/BreakpointManagers/TagBreakpointManager.h
31

Isn't this classof automatically generated by BreakpointBase<TagBreakpoint>.

mlir/lib/Debug/ExecutionContext.cpp
41

shouldn't the logic above be part ActionActiveStack's ctor/dtor ?
ActionActiveStack::ActionActiveStack would only take an action. the rest would automatically handled.

Mogball accepted this revision.Mar 6 2023, 11:42 AM
This revision is now accepted and ready to land.Mar 6 2023, 11:42 AM
rriddle added inline comments.Mar 7 2023, 12:10 AM
mlir/include/mlir/Debug/BreakpointManager.h
36–38

Do you intend to support conditionally enabled breakpoints?

mlir/include/mlir/Debug/BreakpointManagers/TagBreakpointManager.h
28

Why _tag? That doesn't fit with the general naming convention.

Also, std::string -> StringRef?

31

+1, I would expect this to be automatically provided by the CRTP class.

51

Please drop the trivial braces here.

mlir/include/mlir/Debug/ExecutionContext.h
54–55

Can you align the description on the indent? Makes it slightly easier to read.

65

Is the llvm:: necessary here?

mlir/lib/Debug/ExecutionContext.cpp
70–85

Can you remind me what manages the thread safety of all of this?

88

Can you drop this newline?

mlir/unittests/Debug/ExecutionContextTest.cpp
35

Can you sprinkle in some comments as to what these tests are doing?

Also, drop the newline here?

55

Can you drop the trivial braces here?

57–65

This loop looks like it could be cleaned up, and simplified.

91

Why all of the return noOp? These look like they could just be dropped.

179

Is there a cleaner way to check this?

Is there a commit in this stack that handles updating/adding documentation?

mehdi_amini marked 3 inline comments as done.

address comments

mehdi_amini marked 16 inline comments as done.

Address more comments

Is there a commit in this stack that handles updating/adding documentation?

No not yet, I was waiting because I anticipated API changes during the review.

mlir/include/mlir/Debug/BreakpointManager.h
36–38

I would model this with a breakpoint that is "enabled" but has a condition for "matching".

mlir/include/mlir/Debug/BreakpointManagers/TagBreakpointManager.h
31

Did you start this review a while ago and posted it just now? (I addressed this comment already)

mlir/include/mlir/Debug/ExecutionContext.h
109

Actually depth matching is actually how the feature is implemented

mlir/lib/Debug/ExecutionContext.cpp
41

Are you talking about actionStack? It's not clear to me how to handle this in the ActionActiveStack's ctor actually, unless I'd add an extra member. I'd rather not make the class aware of the global variable just now.

70–85

Good point, I added documentation in the header about thread safety aspects of these APIs.

mlir/unittests/Debug/ExecutionContextTest.cpp
179

I can use another counter actually

Is there a commit in this stack that handles updating/adding documentation?

No not yet, I was waiting because I anticipated API changes during the review.

Right now the best reference is likely the ODM on the topic.

rriddle accepted this revision.Mar 8 2023, 10:11 PM
rriddle added inline comments.
mlir/include/mlir/Debug/BreakpointManagers/TagBreakpointManager.h
31

Weird. Maybe I didn't refresh the page, it was still present when I was doing the review.

mlir/lib/Debug/ExecutionContext.cpp
99

Missing newline here.

mlir/unittests/Debug/ExecutionContextTest.cpp
166

Can you drop this noOp?

196

Same here and below.

367

Are any of these necessary?

mehdi_amini marked 6 inline comments as done.

Address comments

This revision was landed with ongoing or failed builds.Mar 12 2023, 2:21 PM
This revision was automatically updated to reflect the committed changes.