This is an archive of the discontinued LLVM Phabricator instance.

[Reland][ADT][ConcurrentHashTable] adapt ConcurrentHashTable and its users to LLVM_ENABLE_THREADS=0 mode.
ClosedPublic

Authored by avl on Apr 5 2023, 11:51 AM.

Details

Summary

This patch hides thread specific handling under LLVM_ENABLE_THREADS guard.
It also removes usages of thread_local variables, since it has a weak
support on some platforms. Instead, the patch uses single mutex for locking
allocator. That may be replaced with more effective allocator later.
f.e. D142318

Diff Detail

Event Timeline

avl created this revision.Apr 5 2023, 11:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2023, 11:51 AM
avl requested review of this revision.Apr 5 2023, 11:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2023, 11:51 AM
SeanP accepted this revision.Apr 5 2023, 12:36 PM

Thanks for the update. I tried this on z/OS and it is working.

This revision is now accepted and ready to land.Apr 5 2023, 12:36 PM
JDevlieghere accepted this revision.Apr 5 2023, 12:42 PM
avl added a comment.Apr 5 2023, 1:15 PM

Thank you for the review.

This revision is now accepted and ready to land.Apr 5 2023, 7:50 PM
avl updated this revision to Diff 511378.Apr 6 2023, 5:20 AM

cure buildbot failure. refactor.

avl retitled this revision from [ADT][ConcurrentHashTable] Change thread_local to LLVM_THREAD_LOCAL inside unit test. to [Reland][ADT][ConcurrentHashTable] adapt ConcurrentHashTable and its users to LLVM_ENABLE_THREADS=0 mode..Apr 6 2023, 5:21 AM
avl edited the summary of this revision. (Show Details)
avl requested review of this revision.EditedApr 6 2023, 5:25 AM

Original patch broke buildbots with memory leak. This new version removes usages of thread_local variables and adds checks for LLVM_ENABLE_THREADS

JDevlieghere accepted this revision.Apr 10 2023, 2:10 PM

It's a bit unfortunate that we have to sprinkle LLVM_ENABLE_THREADS throughout the code, but not enough that I think it's worth doing anything fancy like subclassing (I think it would just make things worse). LGTM.

llvm/include/llvm/DWARFLinkerParallel/StringPool.h
25 ↗(On Diff #511378)

Why not just StringAllocator. Do we expect to have a more "complex" allocator down the line?

This revision is now accepted and ready to land.Apr 10 2023, 2:10 PM
avl added inline comments.Apr 11 2023, 3:37 AM
llvm/include/llvm/DWARFLinkerParallel/StringPool.h
25 ↗(On Diff #511378)

I am planning to use allocator from D142318 later.

Will rename SimpleStringAllocator to StringAllocator as suggested.

SeanP accepted this revision.Apr 11 2023, 8:39 AM

LGTM

I tested this patch on z/os. Everything is working. Thanks