[PATCH] D78324: [Support][X86] Change getHostNumPhsicalCores() to return number of physical cores enabled by affinity

[Alexandre Ganea via Phabricator via llvm-commits]

aganea accepted this revision.
aganea added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/lib/Support/Host.cpp:1303
   int CurCoreId = -1;
   SmallSet<std::pair<int, int>, 32> UniqueItems;
+  for (StringRef Line : strs) {
----------------
MaskRay wrote:
> aganea wrote:
> > Since you're here, it seems really overkill to use a Set to represent a bitfield (also, this will allocate if processors > 32). Can we have `cpu_set_t Enabled` here instead, and use `CPU_COUNT(Enabled)` at the end?
> The idea is that different processor ids may share the same `(physical id, core id)` pair, so we need to count the number of unique `(physical id, core id)` pair.
> 
> We arbitrarily define it as: if one processor id is allowed by the CPU affinity mask, consider its (physical id, core id) available.
Ok. I guess we could read "siblings" and do `CPU_SET(CurPhysicalId * SiblingsId + CurCoreId, &Enabled);` since it's a flat bitfield anyway. Unless we expect asymmetric configurations, with different core count on 2+ CPU sockets, I'm not even sure if that's possible.
Let's leave this for later then if you think it's too much trouble.


================
Comment at: llvm/lib/Support/Host.cpp:1300
   }
   SmallVector<StringRef, 8> strs;
   (*Text)->getBuffer().split(strs, "\n", /*MaxSplit=*/-1,
----------------
One more (almost mandatory) allocation here. :-(
```
6-core:
$ cat /proc/cpuinfo | wc -l
312

18-core:
$ cat /proc/cpuinfo | wc -l
936
```
I know this function is only called once, but all these allocations pile up on the startup time. Can't help seeing this. You don't have to fix it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78324