This is an archive of the discontinued LLVM Phabricator instance.

Add an Observer for logging actions application to a stream
ClosedPublic

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

Details

Summary

Integrate the tracing::ExecutionContext() into mlir-opt with a new
--log-action-to=<file> option to demonstrate the feature.

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
Mogball added inline comments.
mlir/include/mlir/Debug/Observers/ActionLogging.h
21

fill the doc? :P

41

newline

mlir/lib/Debug/Observers/ActionLogging.cpp
21

Is this the canonical way to do something like this? LLVM has no utilities?

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
149

should this emitError?

rriddle added inline comments.Mar 7 2023, 12:16 AM
mlir/include/mlir/Debug/Counter.h
50
mlir/lib/Debug/Observers/ActionLogging.cpp
20

The use of static atomic here is kind of sketchy, why is this necessary?

21

llvm::get_threadid()

mlir/lib/Pass/PassDetail.h
19

Can you shift this to the bottom of the struct, with the other fields?

23–25

I wonder if this would be cleaner using llvm::formatv

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
42

Are these two not already included?

254
mehdi_amini marked 11 inline comments as done.

Address comments

mlir/lib/Debug/Observers/ActionLogging.cpp
20

It was necessary because of thread_local int64_t tid = thread_counter++; is a threaded initialization I believe.

21

Replaced with llvm::get_threadid, but note that it makes the output less deterministic, I kind of liked having threads numbered from 0 to X

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
42

VSCode auto-added includes...

jpienaar added inline comments.Mar 12 2023, 3:55 PM
mlir/include/mlir/Debug/Counter.h
51

OOC why not just isActivated ? (the class is already DebugCounter and above the form is addCounter())

mlir/include/mlir/Debug/Observers/ActionLogging.h
2

Outdated header?

mlir/lib/Debug/Observers/ActionLogging.cpp
2

Same

mlir/lib/Debug/Observers/CMakeLists.txt
2

So this library will contain multiple kind of observers?

mlir/lib/Pass/PassDetail.h
22

Does this have to be inline in header?

22

Mmm, this is quoting using and " , could we just use throughout?

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
144
164

Are these correct destruction order to avoid dangling references ?

rriddle accepted this revision.Mar 12 2023, 11:59 PM

LGTM with Jacques comments addressed

mlir/lib/Debug/Observers/ActionLogging.cpp
21

You can also use llvm::get_thread_name, which can have a more user friendly name for a thread (e.g. the llvm worker threads usually have llvm-worker-<index>.

mlir/lib/Pass/Pass.cpp
466–476

passFailed won't be initialized here if the action doesn't run, this should be set to false.

This revision is now accepted and ready to land.Mar 12 2023, 11:59 PM
mehdi_amini added inline comments.Mar 13 2023, 6:03 AM
mlir/lib/Debug/Observers/ActionLogging.cpp
21

Ah good point! That was added recently to the LLVM thread pool I believe, I'll do that :)

mehdi_amini marked 8 inline comments as done.

Address comment

minor cleanup

clang-format

Minor fix to PAssExecutionAction

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