[PATCH] D142318: [Support] Add PerThreadBumpPtrAllocator class.

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 13 14:17:46 PDT 2023


MaskRay added a comment.

In D142318#4266426 <https://reviews.llvm.org/D142318#4266426>, @avl wrote:

> In D142318#4266388 <https://reviews.llvm.org/D142318#4266388>, @dexonsmith wrote:
>
>> I still have a general concern: this utility isn't safe to use in general LLVM library code, and while that's documented in the header, there's nothing enforcing that or checking for it. I think it'd be easy to get this wrong, and our existing test coverage would be unlikely to catch mistakes, but it could be a big problem for tools/libraries that have their own thread pools and depend on LLVM code.
>>
>> Is there any way of adding assertions, or a structural change, to confirm this hasn't been used in the wrong place? I think it would be okay to add a bit of API surface to ThreadPoolExecutor, or add some new fields under `LLVM_ENABLE_API_BREAKING_CHECKS`.
>
> the possible solution might be initializing threadIndex to some unrelated value by default.
> f.e. setting threadIndex to -1; Threads created by ThreadPoolExecutor would have indexes in range 0 ... ThreadsNum.
> It will trigger assertions "assert(getThreadIndex() < NumOfAllocators);" for wrong threads inside PerThreadAllocator methods. Does it sound OK?

When a `llvm::parallel::parallel_*` function returns, the ThreadPoolExecutor doesn't reset `llvm::parallel::threadIndex`. So the check won't be effective for a `PerThreadBumpPtrAllocator` misuse after a `parallel_*` function call.

Any expensive check mechanism should also support nested `llvm::parallel::parallel_*` calls (even if the inner calls are serial).

I don't have a good approach off my head...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142318



More information about the llvm-commits mailing list