[PATCH] D142317: [Support][LLD] Avoid using uninitialized threadIndex.

[Andrew Ng via Phabricator via llvm-commits]

andrewng added inline comments.


================
Comment at: llvm/lib/Support/Parallel.cpp:231-232
     }
     for (; Begin != End; ++Begin)
       Fn(Begin);
     return;
----------------
avl wrote:
> andrewng wrote:
> > andrewng wrote:
> > > Perhaps a "simpler" fix would be not to use the main thread here, i.e. should use `TG.spawn`? However, that assumes that `getThreadIndex()` is only for use in the "`parallelFor`" style functions otherwise the other parallel functions would also need to avoid using the main thread too.
> > > 
> > > I'm surprised that this hasn't been causing more issues.
> > My gut feeling is that this would be the "better" solution, i.e. not including the invoking thread (might not be the main thread) as part of the pool of worker threads. It just feels cleaner to me.
> > 
> > However, can we also assume that the use of `getThreadIndex()` is not supported with regards to `parallel_sort` which I believe can also make use of the invoking thread? I think this would be reasonable.
> yes, we can avoid using main thread in this case.
> 
> Though, it seems, having the same index for main thread and for one from ThreadPool threads is a potential bug. We would always need to remember to avoid using main thread and ThreadPool threads. Instead, it looks better, to always have different indexes. As this patch does : 0 for main thread, 1... for others.
> 
> How about using TG.spawn in this place, but still having different indexes for main thread and others?
My feeling is that the current `getThreadIndex()` should really only apply in the context of the parallel thread pool threads and the parallel algorithms. Remember there are other thread pools in LLVM too and there may be more threads than just the main one (although perhaps these would just share the same index of `0`?). Other threads may also not be long-lived.

Mixing the concept of the parallel `getThreadIndex()` with other threads just doesn't feel right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142317