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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

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

> 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?

Sounds reasonable to me. Maybe we'd want to keep zero-init for non-assertions builds to avoid unnecessary static initializers?

In D142318#4266443 <https://reviews.llvm.org/D142318#4266443>, @MaskRay wrote:

> 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.

I don't think we need to catch every possible failure. It sounds useful (even if not exhaustive) to catch misuse in the cases where no `llvm::parallel` code has been called yet. Then, regular unit test coverage for such library code can trigger assertion failures....

> Any expensive check mechanism

Note, `LLVM_ABI_BREAKING_CHECKS` (spelling?) is on-by-default in assertions builds. This is different from `LLVM_EXPENSIVE_CHECKS` (sp?). I think if we can't make the assertion cheap enough to be on-by-default in assertions builds it probably isn't worth it.

(Not sure @avl's suggestion is ABI-breaking anyway so it's probably moot.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142318