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

[Lang Hames via Phabricator via llvm-commits]

lhames marked 2 inline comments as done.
lhames added inline comments.


================
Comment at: compiler-rt/lib/orc/debug.h:33
+    if (::__orc_rt::DebugState == DebugStates::Uninitialized)                  \
+      initializeDebug();                                                       \
+    if (::__orc_rt::DebugState == DebugStates::Enabled &&                      \
----------------
benlangmuir wrote:
> lhames wrote:
> > 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? 
> Okay, I'm not going to try to hold this patch up over this, but I would suggest at least refactoring to only do a single atomic load in the common case that you've already done the initialization, and preferably using acquire or relaxed ordering.
> 
> > 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?
> 
> I suspect the slowness of the logging itself would be a bigger issue for reproducibility of bugs unless you're running under tsan.  Or are you thinking of when logging is disabled? 
I was thinking when logging is disabled, which will be common during development. My thought was that maybe this would matter if we ever wanted to try turning on logging in release mode to find a bug. It's too early to worry about that though -- it's not clear if/when we'll ever need that, nor how much this would help.

That said, I've switched to relaxed everywhere because it doesn't seem like it should hurt: repeated initialize calls will always write the same values, so at worst we'll duplicate some work on multiple threads at initialization time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116139