[PATCH] D92419: [Support] On Windows, take the affinity mask into account

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added inline comments.


================
Comment at: llvm/lib/Support/Windows/Threading.inc:200
+    // the current process to only a single CPU group. On Windows, it is not
+    // possible for affinity masks to cross CPU groups boundaries.
+    DWORD_PTR ProcessAffinityMask = 0, SystemAffinityMask = 0;
----------------
nit:  In "group boundaries", _group_ should be singular.


================
Comment at: llvm/unittests/Support/ThreadPool.cpp:178
+    ThreadPoolStrategy S,
+    llvm::function_ref<void(llvm::DenseSet<llvm::BitVector> &)> Fn) {
   // FIXME: Skip these tests on non-Windows because multi-socket system were not
----------------
I think the generality introduced here may be more complexity than it's worth.  It imposes a high boilerplate cost on each caller.

Both tests that currently call this function simply look at the size (population count) of the bit vector.  Unless you expect to use this in more ways, it would be simpler to return a copy of the bit vector (or just its size) and let the individual tests inspect it.  That would still move the assertions to the individual tests, which is valuable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92419