Add interface to compute number of physical cores on host system

[tejohnson]

Summary

For now I have only added support for x86_64 Linux, but other systems
can be added incrementally.

This is to be used for setting the default parallelism for ThinLTO
backends (instead of thread::hardware_concurrency which includes
hyperthreading and is too aggressive). I'll send this as a follow-on
patch, and it will fall back to hardware_concurrency when the new
getHostNumPhysicalCores returns -1 (when not supported for a given
host system).

I also added an interface to MemoryBuffer to force reading a file
as a stream - this is required for /proc/cpuinfo which is a special
file that looks like a normal file but appears to have 0 size.
The existing readers of this file in Host.cpp are reading the first
1024 or so bytes from it, because the necessary info is near the top.
But for the new functionality we need to be able to read the entire
file. I can go back and change the other readers to use the new
getFileAsStream as a follow-on patch since it seems much more robust.

Added a unittest.

[mehdi_amini]

lib/Support/Host.cpp

1197 ↗

(On Diff #74526)

I think we should read the file once:

static int computeHostNumPhysicalCores() {
  // all your stuff
}
int sys::getHostNumPhysicalCores() {
  static int NumCores = computeHostNumPhysicalCores();
  return NumCores;
}

1212 ↗

(On Diff #74526)

Not that it matters that much, but have you considered reading the file line by line instead of reading it as whole, splitting the lines, and then processing them in memory?

unittests/Support/Host.cpp

29 ↗

(On Diff #74526)

May want to check tuple <OS, arch>, but that can be changed when it'll be needed.

[tejohnson]

lib/Support/Host.cpp

1197 ↗

(On Diff #74526)

Good idea, done.

1212 ↗

(On Diff #74526)

I originally thought about that when I saw that Host.cpp already rolled its own support for reading in (part of) /proc/cpuinfo that didn't meet my needs. But I felt that using the existing memory buffer streaming support (already used if we detect this isn't a regular file, or for stdin) was cleaner and simpler.

unittests/Support/Host.cpp

29 ↗

(On Diff #74526)

Good idea - done. This also made me realize it should be an AND condition not an OR!

[tejohnson]

Address review comments

[mehdi_amini]

LGTM.

unittests/Support/Host.cpp

29 ↗

(On Diff #74526)

Yes that's what I had in mind :)

[aaron.ballman]

lib/Support/Host.cpp

1239 ↗

(On Diff #74537)

No Windows implementation? Should be able to use GetLogicalProcessorInformation() pretty easily.

https://msdn.microsoft.com/en-us/library/windows/desktop/ms683194(v=vs.85).aspx

[mehdi_amini]

lib/Support/Host.cpp

1239 ↗

(On Diff #74537)

Patch welcome :)

I'll supply the MacOS version when possible.

[tejohnson]

lib/Support/Host.cpp

1239 ↗

(On Diff #74537)

I'd prefer someone who has access to Windows for pre-commit testing to contribute a patch. I'd have to guess and just wait for bots to test. (Mehdi had already committed to supplying the Mac OS side patch.)

[kparzysz]

Why specifically asking about physical cores? Should this ask instead about the preferred number of concurrent tasks?

[mehdi_amini]

In D25564#569453, @kparzysz wrote:

Why specifically asking about physical cores? Should this ask instead about the preferred number of concurrent tasks?

The reason is that if you have for instance very memory heavy tasks, and you know (by experiment) that you don't benefit much from hyper-threading in terms of runtime, it seems more friendly to kick less threads (divide memory peak by 2).

[mehdi_amini]

In D25564#569457, @mehdi_amini wrote:

In D25564#569453, @kparzysz wrote:

Why specifically asking about physical cores? Should this ask instead about the preferred number of concurrent tasks?

Note also, we already have llvm::thread::hardware_concurrency() that returns the "concurrent tasks" (i.e. virtual cores on machines with hyper-threading).

[kparzysz]

In D25564#569457, @mehdi_amini wrote:

The reason is that if you have for instance very memory heavy tasks, and you know (by experiment) that you don't benefit much from hyper-threading in terms of runtime, it seems more friendly to kick less threads (divide memory peak by 2).

Then what's stopping us from returning that number as "preferred number of concurrent tasks"? Instead we have something motivated by x86 that may or may not work well on other architectures.

[mehdi_amini]

In D25564#569461, @kparzysz wrote:

In D25564#569457, @mehdi_amini wrote:

The reason is that if you have for instance very memory heavy tasks, and you know (by experiment) that you don't benefit much from hyper-threading in terms of runtime, it seems more friendly to kick less threads (divide memory peak by 2).

Then what's stopping us from returning that number as "preferred number of concurrent tasks"?

Do you mean having llvm::thread::hardware_concurrency()returning the "preferred number of concurrent tasks"?
This does not seem a good idea to me, because there was an important "if" in the explanation above: "if you have for instance very memory heavy tasks, and you know (by experiment) that you don't benefit much from hyper-threading in terms of runtime".

Instead we have something motivated by x86 that may or may not work well on other architectures.

I'm not sure how to solve it differently than delegating such logic to the client of this API, i.e. I see this API is a basic-block to build other APIs.
This other API could be for instance llvm::thread::hardware_coarse_concurrency(). This is where I would handle handle architecture specific choices (and default to hardware_concurrency).

[kparzysz]

In D25564#569547, @mehdi_amini wrote:

I'm not sure how to solve it differently than delegating such logic to the client of this API, i.e. I see this API is a basic-block to build other APIs.
This other API could be for instance llvm::thread::hardware_coarse_concurrency(). This is where I would handle handle architecture specific choices (and default to hardware_concurrency).

The host will most often be x86, so this isn't really that much of a big deal. It just seemed like a cleaner idea would be to query the OS about how many concurrent tasks it could handle at the moment, and that could transparently take into account the current load on the machine, if someone wanted to go that far.

llvm/trunk/include/llvm/Support/Host.h

#endif
   ///
   /// \param Features - A string mapping feature names to either
   /// true (if enabled) or false (if disabled). This routine makes no guarantees
   /// about exactly which features may appear in this map, except that they are
   /// all valid LLVM feature names.
   ///
   /// \return - True on success.
   bool getHostCPUFeatures(StringMap<bool> &Features);
+
+  /// Get the number of physical cores (as opposed to logical cores returned
+  /// from thread::hardware_concurrency(), which includes hyperthreads).
+  /// Returns -1 if unknown for the current host system.
+  int getHostNumPhysicalCores();
 }
 }
 
 #endif

llvm/trunk/include/llvm/Support/MemoryBuffer.h

public:
   ///
   /// \param IsVolatileSize Set to true to indicate that the file size may be
   /// changing, e.g. when libclang tries to parse while the user is
   /// editing/updating the file.
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getFile(const Twine &Filename, int64_t FileSize = -1,
           bool RequiresNullTerminator = true, bool IsVolatileSize = false);
 
+  /// Read all of the specified file into a MemoryBuffer as a stream
+  /// (i.e. until EOF reached). This is useful for special files that
+  /// look like a regular file but have 0 size (e.g. /proc/cpuinfo on Linux).
+  static ErrorOr<std::unique_ptr<MemoryBuffer>>
+  getFileAsStream(const Twine &Filename);
+
   /// Given an already-open file descriptor, map some slice of it into a
   /// MemoryBuffer. The slice is specified by an \p Offset and \p MapSize.
   /// Since this is in the middle of a file, the buffer is not null terminated.
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getOpenFileSlice(int FD, const Twine &Filename, uint64_t MapSize,
                    int64_t Offset);
 
   /// Given an already-open file descriptor, read the file and return a

llvm/trunk/lib/Support/Host.cpp

 //===-- Host.cpp - Implement OS Host Concept --------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements the operating system Host concept.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Host.h"
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/raw_ostream.h"
-#include <string.h>
 #include <assert.h>
+#include <string.h>
 
 // Include the platform-specific parts of this class.
 #ifdef LLVM_ON_UNIX
 #include "Unix/Host.inc"
 #endif
 #ifdef LLVM_ON_WIN32
 #include "Windows/Host.inc"
 #endif
StringRef sys::getHostCPUName() {
   }
 
   return "generic";
 }
 #else
 StringRef sys::getHostCPUName() { return "generic"; }
 #endif
 
+#if defined(__linux__) && defined(__x86_64__)
+// On Linux, the number of physical cores can be computed from /proc/cpuinfo,
+// using the number of unique physical/core id pairs. The following
+// implementation reads the /proc/cpuinfo format on an x86_64 system.
+int computeHostNumPhysicalCores() {
+  // Read /proc/cpuinfo as a stream (until EOF reached). It cannot be
+  // mmapped because it appears to have 0 size.
+  llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Text =
+      llvm::MemoryBuffer::getFileAsStream("/proc/cpuinfo");
+  if (std::error_code EC = Text.getError()) {
+    llvm::errs() << "Can't read "
+                 << "/proc/cpuinfo: " << EC.message() << "\n";
+  }
+  SmallVector<StringRef, 8> strs;
+  (*Text)->getBuffer().split(strs, "\n", /*MaxSplit=*/-1,
+                             /*KeepEmpty=*/false);
+  int CurPhysicalId = -1;
+  int CurCoreId = -1;
+  SmallSet<std::pair<int, int>, 32> UniqueItems;
+  for (auto &Line : strs) {
+    Line = Line.trim();
+    if (!Line.startswith("physical id") && !Line.startswith("core id"))
+      continue;
+    std::pair<StringRef, StringRef> Data = Line.split(':');
+    auto Name = Data.first.trim();
+    auto Val = Data.second.trim();
+    if (Name == "physical id") {
+      assert(CurPhysicalId == -1 &&
+             "Expected a core id before seeing another physical id");
+      Val.getAsInteger(10, CurPhysicalId);
+    }
+    if (Name == "core id") {
+      assert(CurCoreId == -1 &&
+             "Expected a physical id before seeing another core id");
+      Val.getAsInteger(10, CurCoreId);
+    }
+    if (CurPhysicalId != -1 && CurCoreId != -1) {
+      UniqueItems.insert(std::make_pair(CurPhysicalId, CurCoreId));
+      CurPhysicalId = -1;
+      CurCoreId = -1;
+    }
+  }
+  return UniqueItems.size();
+}
+#else
+// On other systems, return -1 to indicate unknown.
+int computeHostNumPhysicalCores() { return -1; }
+#endif
+
+int sys::getHostNumPhysicalCores() {
+  static int NumCores = computeHostNumPhysicalCores();
+  return NumCores;
+}
+
 #if defined(__i386__) || defined(_M_IX86) || \
     defined(__x86_64__) || defined(_M_X64)
 bool sys::getHostCPUFeatures(StringMap<bool> &Features) {
   unsigned EAX = 0, EBX = 0, ECX = 0, EDX = 0;
   unsigned MaxLevel;
   union {
     unsigned u[3];
     char c[12];

llvm/trunk/lib/Support/MemoryBuffer.cpp

ErrorOr<std::unique_ptr<MemoryBuffer>> MemoryBuffer::getSTDIN() {
   //
   // FIXME: That isn't necessarily true, we should try to mmap stdin and
   // fallback if it fails.
   sys::ChangeStdinToBinary();
 
   return getMemoryBufferForStream(0, "<stdin>");
 }
 
+ErrorOr<std::unique_ptr<MemoryBuffer>>
+MemoryBuffer::getFileAsStream(const Twine &Filename) {
+  int FD;
+  std::error_code EC = sys::fs::openFileForRead(Filename, FD);
+  if (EC)
+    return EC;
+  ErrorOr<std::unique_ptr<MemoryBuffer>> Ret =
+      getMemoryBufferForStream(FD, Filename);
+  close(FD);
+  return Ret;
+}
+
 MemoryBufferRef MemoryBuffer::getMemBufferRef() const {
   StringRef Data = getBuffer();
   StringRef Identifier = getBufferIdentifier();
   return MemoryBufferRef(Data, Identifier);
 }

llvm/trunk/unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   ConvertUTFTest.cpp
   DataExtractorTest.cpp
   DwarfTest.cpp
   EndianStreamTest.cpp
   EndianTest.cpp
   ErrorTest.cpp
   ErrorOrTest.cpp
   FileOutputBufferTest.cpp
+  Host.cpp
   LEB128Test.cpp
   LineIteratorTest.cpp
   LockFileManagerTest.cpp
   MD5Test.cpp
   ManagedStatic.cpp
   MathExtrasTest.cpp
   MemoryBufferTest.cpp
   MemoryTest.cpp

llvm/trunk/unittests/Support/Host.cpp

+//========- unittests/Support/Host.cpp - Host.cpp tests --------------========//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/Host.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/Triple.h"
+
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+class HostTest : public testing::Test {
+  Triple Host;
+  SmallVector<std::pair<Triple::ArchType, Triple::OSType>, 4> SupportedArchAndOSs;
+
+protected:
+  bool isSupportedArchAndOS() {
+    if (is_contained(SupportedArchAndOSs, std::make_pair(Host.getArch(), Host.getOS())))
+      return true;
+
+    return false;
+  }
+
+  HostTest() {
+    Host.setTriple(Triple::normalize(sys::getProcessTriple()));
+
+    // Initially this is only testing detection of the number of
+    // physical cores, which is currently only supported for
+    // x86_64 Linux.
+    SupportedArchAndOSs.push_back(std::make_pair(Triple::x86_64, Triple::Linux));
+  }
+};
+
+TEST_F(HostTest, NumPhysicalCores) {
+  int Num = sys::getHostNumPhysicalCores();
+
+  if (isSupportedArchAndOS())
+    ASSERT_GT(Num, 0);
+  else
+    ASSERT_EQ(Num, -1);
+}