Page MenuHomePhabricator

Fix computeHostNumPhysicalCores() for Linux on POWER and Linux on Z
ClosedPublic

Authored by etiotto on Jul 28 2020, 8:56 AM.

Details

Summary

ThinLTO is run using a single thread on Linux on Power. The compute_thread_count() routine calls getHostNumPhysicalCores which returns -1 by default, and so `MaxThreadCount is set to 1.

unsigned llvm::ThreadPoolStrategy::compute_thread_count() const {
   
    int MaxThreadCount = UseHyperThreads ? computeHostNumHardwareThreads()
                                        : sys::getHostNumPhysicalCores();
     if (MaxThreadCount <= 0)
        MaxThreadCount = 1;
   …
}

Fix: provide custom implementation of getHostNumPhysicalCores for Linux on Power and Linux on Z.

Diff Detail

Unit TestsFailed

TimeTest
180 mslinux > Polly.ScopInfo::memcpy-raw-source.ll
Script: -- : 'RUN: at line 1'; opt -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/mnt/disks/ssd0/agent/llvm-project/polly/test/ScopInfo -polly-codegen-verify -basic-aa -scoped-noalias -tbaa -polly-scops -analyze < /mnt/disks/ssd0/agent/llvm-project/polly/test/ScopInfo/memcpy-raw-source.ll

Event Timeline

etiotto created this revision.Jul 28 2020, 8:56 AM
etiotto requested review of this revision.Jul 28 2020, 8:56 AM
RKSimon resigned from this revision.Jul 28 2020, 9:08 AM
Kai accepted this revision.Jul 28 2020, 11:21 AM

LGTM.

This revision is now accepted and ready to land.Jul 28 2020, 11:21 AM
etiotto updated this revision to Diff 281566.Jul 29 2020, 7:07 AM

Fix formatting

jsji added a subscriber: jsji.

LoZ.

uweigand accepted this revision.Jul 29 2020, 10:38 AM

LGTM for SystemZ as well.

etiotto updated this revision to Diff 281730.Jul 29 2020, 1:58 PM

Fix test case for PPC64 and SystemZ

@uweigand and @Kai I forgot to fix a test case. Now fixed are you still ok with the test case change?

Sure, of course assuming the test still passes.

Fix: provide custom implementation of getHostNumPhysicalCores for Linux on Power and Linux on Z.

computeHostNumPhysicalCores() is designed to respect CPU affinity.
SupportTests Threading.PhysicalConcurrency may fail if taskset -c is specified.

I fixed the test in cd53ded557c3487b8dae2f9de894fdb5b75cb8c8. Please refer to the x86 implementation if you want to support both hyper threading & affinity.

llvm/lib/Support/Host.cpp
1275

defined(__ppc__) || defined(__powerpc__) is redundant.

You could just use defined(__powerpc__)

llvm/unittests/Support/Host.cpp
45 ↗(On Diff #281992)

Did you want to enable powerpc32 as well?

etiotto marked 2 inline comments as done.Jul 31 2020, 1:55 PM
etiotto added inline comments.
llvm/lib/Support/Host.cpp
1275

Other PPC code already uses both. I used both for consistency.

llvm/unittests/Support/Host.cpp
45 ↗(On Diff #281992)

No

etiotto marked 2 inline comments as done.Jul 31 2020, 1:55 PM
MaskRay added inline comments.Jul 31 2020, 2:24 PM
llvm/lib/Support/Host.cpp
1275

This seems like cargo culting. One macro is sufficient.

llvm/unittests/Support/Host.cpp
45 ↗(On Diff #281992)

If no, the macro you should have used is __powerpc64__

RKSimon added a subscriber: RKSimon.Aug 4 2020, 6:40 AM

@etiotto @MaskRay This has been failing for a number of days:

http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/12748
http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage

Please can somebody take a look and either revert the patch or fix it?