Integrate the tracing::ExecutionContext() into mlir-opt with a new
--log-action-to=<file> option to demonstrate the feature.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 |
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... |
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 | Error form should be sentence fragments (https://llvm.org/docs/CodingStandards.html#error-and-warning-messages) | |
164 | Are these correct destruction order to avoid dangling references ? |
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. |
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 :) |