This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by MaskRay on Apr 16 2020, 1:21 PM.

Details

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=45556

While here, make the x86-64 code available for x86-32.

The output has been available and stable since
https://git.kernel.org/torvalds/c/3dd9d514846cdca1dcef2e4fce666d85e199e844 (2005)

processor:
...
physical id:
siblings:
core id:

Don't check HAVE_SCHED_GETAFFINITY/HAVE_CPU_COUNT. The interface is
simply available in every libc which can build LLVM.

Diff Detail

Event Timeline

MaskRay created this revision.Apr 16 2020, 1:21 PM

Check if this fixes PR45556; also if it works with -DLLVM_ENABLE_THREADS=OFF (it should, but who knows).

llvm/lib/Support/Host.cpp
1294

cpu_set_t Affinity; ?

1307–1308

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?

1322

CPU_SET(CurProcessor, Enabled); as suggested above?

MaskRay updated this revision to Diff 258170.Apr 16 2020, 2:12 PM
MaskRay marked 3 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

Rename Set to Affinity
Add comments.
Mention https://bugs.llvm.org/show_bug.cgi?id=45556

llvm/lib/Support/Host.cpp
1307–1308

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.

aganea accepted this revision.Apr 16 2020, 3:12 PM

LGTM.

llvm/lib/Support/Host.cpp
1301

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.

1307–1308

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.

This revision is now accepted and ready to land.Apr 16 2020, 3:12 PM
MaskRay updated this revision to Diff 258201.Apr 16 2020, 4:32 PM
MaskRay marked 2 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

Address comments

aganea accepted this revision.Apr 16 2020, 6:35 PM
aganea added subscribers: dexonsmith, john.brawn.

Thanks for doing this, still looking good.

So to be complete, only Darwin remains, if ever. @dexonsmith

This revision was automatically updated to reflect the committed changes.