[PATCH] D25564: Add interface to compute number of physical cores on host system

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Support/Host.cpp:1197
+// implementation reads the /proc/cpuinfo format on an x86_64 system.
+int sys::getHostNumPhysicalCores() {
+  // Read /proc/cpuinfo as a stream (until EOF reached). It cannot be
----------------
mehdi_amini wrote:
> I think we should read the file once:
> 
> ```
> static int computeHostNumPhysicalCores() {
>   // all your stuff
> }
> int sys::getHostNumPhysicalCores() {
>   static int NumCores = computeHostNumPhysicalCores();
>   return NumCores;
> }
> ```
Good idea, done.


================
Comment at: lib/Support/Host.cpp:1212
+  SmallSet<std::pair<int, int>, 32> UniqueItems;
+  for (auto &Line : strs) {
+    Line = Line.trim();
----------------
mehdi_amini wrote:
> 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?
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.


================
Comment at: unittests/Support/Host.cpp:29
+    if (is_contained(SupportedArchs, Host.getArch()))
+      return true;
+
----------------
mehdi_amini wrote:
> May want to check tuple `<OS, arch>`, but that can be changed when it'll be needed.
Good idea - done. This also made me realize it should be an AND condition not an OR!


https://reviews.llvm.org/D25564