[PATCH] D116139: [ORC-RT] Add debug logging macros.
Lang Hames via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 23 01:55:55 PST 2021
lhames added inline comments.
================
Comment at: compiler-rt/lib/orc/debug.cpp:28
+ DebugTypes = DT;
+ if (getenv("ORC_RT_DEBUG"))
+ DebugState = DebugStates::Enabled;
----------------
benlangmuir wrote:
> I'm not familiar with the history of `-debug` vs. `-debug-only`. What's the reason for not just basing this on a single value -- is `ORC_RT_DEBUG_TYPES` empty or not?
Using the empty-string feels counter-intuitive -- I think people are going to write:
`ORC_RT_DEBUG=1` and get confused when it doesn't print anything.
What if we reserve '1', 'true', 'on', or 'all' for "log everything"?
================
Comment at: compiler-rt/lib/orc/debug.h:33
+ if (::__orc_rt::DebugState == DebugStates::Uninitialized) \
+ initializeDebug(); \
+ if (::__orc_rt::DebugState == DebugStates::Enabled && \
----------------
benlangmuir wrote:
> If we really need to initialize it here, we could skip the second atomic load in the common case that it's already initialized
>
> ```
> auto State = ::__orc_rt::DebugState.load(memory_order_acquire);
> if (State == DebugStates::Uninitialized) {
> initializeDebug();
> State = ::__orc_rt::DebugState.load(memory_order_relaxed);
> }
> if (State == DebugStates::Enabled && ...
> ```
>
> Or to improve code size, initialize `DebugState` to `Enabled` and put a `std::call_once` in `isCurrentDebugType` to actually initialize it the first time.
>
> We could also get away with `load(memory_order_acquire)` here instead of the default sequentially consistent. Or perhaps even `load(memory_order_relaxed)` if we're willing to accept a stale value of DebugTypes if there's contention during initialization.
It's a debug macro and the ORC runtime isn't likely to contain any too-hot code. I guess one think that we need to watch out for is the possibility that relying on sequential consistency for logging checks might obscure ordering bugs?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116139/new/
https://reviews.llvm.org/D116139
More information about the llvm-commits
mailing list