rnk added subscribers: inglorion, gbiv.
rnk added a comment.
Herald added a subscriber: george.burgess.iv.
+@gbiv @inglorion, other ThinLTO users.
> To solve this situation, we capture (and retain) the initial intention until
> the point of usage, through a new ThreadPoolStrategy class. The number of
> threads to use is deferred as late as possible, until the moment where the
> std::threads are created (ThreadPool in the case of ThinLTO).
That seems reasonable to me.
================
Comment at: llvm/include/llvm/ADT/SmallBitVector.h:487
+
+ int compare(const SmallBitVector &RHS) const {
+ for (unsigned I = 0; I < std::max(size(), RHS.size()); ++I) {
----------------
I guess we don't need these changes, since it looks like you use BitVector only
below.
================
Comment at: llvm/include/llvm/ADT/SmallBitVector.h:500
+
+ bool operator<(const SmallBitVector &RHS) const { return compare(RHS) == -1;
}
+
----------------
You return `A - B` but then compare for equality to -1 and 1. I guess it works
because you are doing it bit by bit, but it's exciting.
================
Comment at: llvm/include/llvm/Support/Threading.h:153
+ // runtime might use a lower value (not higher).
+ unsigned ThreadCount = 0;
+
----------------
Let's name the fields in a way that indicates that these numbers are the
requested number of threads, not the final number. So, `ThreadsRequested` or
something like that.
================
Comment at: llvm/include/llvm/Support/Threading.h:158
+ // specific core).
+ bool HyperThreads = true;
+
----------------
This could be `UseHyperThreads`. The first time I read it, I guessed that it
indicated if the system has hyperthreads.
================
Comment at: llvm/lib/Support/Host.cpp:1277
<< "/proc/cpuinfo: " << EC.message() << "\n";
return -1;
}
----------------
Another -1 case.
================
Comment at: llvm/lib/Support/Host.cpp:1334
// On other systems, return -1 to indicate unknown.
static int computeHostNumPhysicalCores() { return -1; }
#endif
----------------
Note: the -1 case.
================
Comment at: llvm/lib/Support/Threading.cpp:85-86
-#include <thread>
-unsigned llvm::heavyweight_hardware_concurrency() {
- // Since we can't get here unless LLVM_ENABLE_THREADS == 1, it is safe to use
- // `std::thread` directly instead of `llvm::thread` (and indeed, doing so
- // allows us to not define `thread` in the llvm namespace, which conflicts
- // with some platforms such as FreeBSD whose headers also define a struct
- // called `thread` in the global namespace which can cause ambiguity due to
- // ADL.
- int NumPhysical = sys::getHostNumPhysicalCores();
- if (NumPhysical == -1)
- return std::thread::hardware_concurrency();
- return NumPhysical;
-}
+int computeHostNumPhysicalCores();
+int computeHostNumPhysicalThreads();
----------------
We already have a public API, getHostNumPhysicalCores. Can this use that?
================
Comment at: llvm/lib/Support/Threading.cpp:89
+unsigned llvm::ThreadPoolStrategy::compute_thread_count() const {
+ unsigned MaxThreadCount = HyperThreads ? computeHostNumPhysicalThreads()
+ : computeHostNumPhysicalCores();
----------------
I see these `computeHostNum*` methods return int, and some versions seem to
return -1 to indicate errors. I think you'll want to use `int` here and check
if `MaxThreadCount <= 0`. Otherwise below we may do a signed/unsigned
comparison mismatch, and return ~0U for MaxThreadCount.
================
Comment at: llvm/lib/Support/Unix/Threading.inc:273
+
+int computeHostNumPhysicalThreads() {
+#if defined(HAVE_SCHED_GETAFFINITY) && defined(HAVE_CPU_COUNT)
----------------
I'm not sure it makes sense to say "physical threads". I think "physical cores"
was meant to distinguish between hardware threads and cores. Describing
hardware execution resources as physical or non-physical isn't very precise or
meaningful in the first place, but I don't think it should apply to hyper
threads.
================
Comment at: llvm/lib/Support/Windows/Threading.inc:160
+
+static ArrayRef<ProcessorGroup> computeProcessorGroups() {
+ auto computeGroups = []() {
----------------
This is cached, so maybe `getProcessorGroups` to indicate that it is not
intended to be expensive.
================
Comment at: llvm/lib/Support/Windows/Threading.inc:212
+template <typename R, typename UnaryPredicate>
+unsigned aggregate(R &&Range, UnaryPredicate P) {
+ unsigned I{};
----------------
Seems like `static` can be used here.
================
Comment at: llvm/unittests/Support/ThreadPool.cpp:31
protected:
// This is intended for platform as a temporary "XFAIL"
bool isUnsupportedOSOrEnvironment() {
----------------
"Temporary" >_>
================
Comment at: llvm/unittests/Support/ThreadPool.cpp:180
+
+ //std::set<llvm::BitVector> ThreadsUsed;
+ llvm::DenseSet<llvm::BitVector> ThreadsUsed;
----------------
I guess this is why you added comparison operators. In any case, let's remove
the commented out code in the final version.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71775/new/
https://reviews.llvm.org/D71775
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
