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

[Ben Langmuir via Phabricator via llvm-commits]

benlangmuir added inline comments.


================
Comment at: compiler-rt/lib/orc/debug.cpp:28
+    DebugTypes = DT;
+  if (getenv("ORC_RT_DEBUG"))
+    DebugState = DebugStates::Enabled;
----------------
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?


================
Comment at: compiler-rt/lib/orc/debug.cpp:49
+        return false;
+      if (*End == ',')
+        Start = End + 1;
----------------
This could be an assert instead of a condition.


================
Comment at: compiler-rt/lib/orc/debug.h:33
+    if (::__orc_rt::DebugState == DebugStates::Uninitialized)                  \
+      initializeDebug();                                                       \
+    if (::__orc_rt::DebugState == DebugStates::Enabled &&                      \
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116139/new/

https://reviews.llvm.org/D116139