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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

I also have a connected question not related directly. What do you think about allocator from D142318 <https://reviews.llvm.org/D142318> and following design for such allocator:

  class ThreadPoolAllocator {
    void *Allocate(size_t size, size_t alignment);
  };
  
  class ThreadPool {
    std::unique_ptr<ThreadPoolAllocator> getAllocator ();
  };
  
  class ThreadPoolExecutor {
    std::unique_ptr<ThreadPoolAllocator> getAllocator ();
  };

The idea is to have thread-safe allocator connected with its execution context.



================
Comment at: llvm/lib/Support/Parallel.cpp:231-232
     }
     for (; Begin != End; ++Begin)
       Fn(Begin);
     return;
----------------
andrewng wrote:
> 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.
I see. To summaries the changes :

1. remove zero initialization.
2. remove advancing index for zero thread.
3. use  TG.spawn  instead of main thread.

correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142317