[PATCH] D85531: [SystemZ/ZOS] Add support for getHostNumPhysicalCores()

Mon Aug 10 08:09:48 PDT 2020

hubert.reinterpretcast added inline comments.

================
Comment at: llvm/lib/Support/Host.cpp:1334
+      static_cast<uintptr_t>(reinterpret_cast<unsigned int &>(CVT[CVTCSD])));
+  return reinterpret_cast<int &>(CSD[CSD_NUMBER_ONLINE_STANDARD_CPS]);
+}
----------------
aganea wrote:
> hubert.reinterpretcast wrote:
> > Kai wrote:
> > > MaskRay wrote:
> > > > If z/OS supports something similar to `taskset -c 0-3 $program`, make sure the function returns a number less than or equal to 4. 
> > > Thanks for the hint. This kind of command is not supported on z/OS.
> > > The reason is the fault tolerance available in z/OS: if a defect in a CP is detected, then this CP can go offline and a spare CP can continue to run the code exactly at the point where the former CP went offline. This does not work if the process is tied to a certain set of CPs, and therefore it's not supported.
> > @MaskRay, I think the test you propose imposes an additional restriction that I am not sure the existing implementations of the function adhere to. Namely, I believe the test that you proposed indicates that the intent is to return a number that is one greater than the highest processor index number on the system. I think this might differ from the count of available processors. My understanding is that the function is meant to return the latter.
> @hubert.reinterpretcast Actually both the Linux & Windows implementations return the restricted set of CPUs/processors that are usable within the process (ie. CPUs represented by the affinity mask).
> https://github.com/llvm/llvm-project/blob/62a933b72c5b060bcb2c7332d05082f002d6c65a/llvm/lib/Support/Host.cpp#L1234
The restricted set is what I meant by "available". In other words, I understand that the current implementations returns what you describe.

My comment was on the relationship between the count and the index. If the first usable index is higher than the count, then the command MaskRay proposed would fail.

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

https://reviews.llvm.org/D85531