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
22

fill the doc? :P

42

newline

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

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

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

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
21

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

22

llvm::get_threadid()

mlir/lib/Pass/PassDetail.h
20

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

24–26

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

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

Are these two not already included?

259
mehdi_amini marked 11 inline comments as done.

Address comments

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

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

22

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
1

Outdated header?

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

Same

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

So this library will contain multiple kind of observers?

mlir/lib/Pass/PassDetail.h
23

Does this have to be inline in header?

23

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

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
147
167

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
22

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

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
22

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.