[PATCH] D116139: [ORC-RT] Add debug logging macros.

[Lang Hames via Phabricator via llvm-commits]

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