This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ/ZOS] Add support for getHostNumPhysicalCores()
ClosedPublic

Authored by Kai on Aug 7 2020, 8:45 AM.

Details

Summary

The information about the CPs online in an LPAR is stored in a control block.
The code navigates to the right control block and returns the number.

Diff Detail

Event Timeline

Kai created this revision.Aug 7 2020, 8:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2020, 8:45 AM
Kai requested review of this revision.Aug 7 2020, 8:45 AM
llvm/unittests/Support/Host.cpp
40

Minor nit: grammar

llvm/unittests/Support/Host.cpp
40

The x86_64 part of the sentence is meant to be distributed across "Linux" and "Darwin" and not to z/OS. With the new formulation, x86_64 needs to be added to before "Darwin" to retain the intended meaning.

46

Minor nit: Aside for Windows, where the expression here is not sensitive to the platform architecture, can we structure this to consistently group by architecture? Alternatively, consistently group by OS.

MaskRay added inline comments.Aug 8 2020, 9:28 PM
llvm/lib/Support/Host.cpp
1334

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.

Kai added inline comments.Aug 10 2020, 3:11 AM
llvm/lib/Support/Host.cpp
1334

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.

Kai updated this revision to Diff 284318.Aug 10 2020, 5:06 AM
  • Sorted condition according to OS
  • Updated/fixed comment
Kai marked 2 inline comments as done.Aug 10 2020, 5:08 AM
Kai added inline comments.
llvm/unittests/Support/Host.cpp
40

I hope the new order is better.
As alternative I also thought about removing this part of the comment, as it only rephrases the condition without giving new information.

LGTM with minor comment.

llvm/unittests/Support/Host.cpp
40

I think the comment doesn't need to indicate the list (note that the duplication led to Windows being in one list and not ther other). The comment does help in explaining what the function does though.

This revision is now accepted and ready to land.Aug 10 2020, 7:27 AM
llvm/lib/Support/Host.cpp
1334

@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.

aganea added inline comments.Aug 10 2020, 7:56 AM
llvm/lib/Support/Host.cpp
1334

@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

llvm/lib/Support/Host.cpp
1334

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.

aganea added inline comments.Aug 10 2020, 8:18 AM
llvm/lib/Support/Host.cpp
1334

I think @MaskRay was only suggesting to take the affinity mask into account. On Linux you could say taskset -c 6-8,10,15 and in that case computeHostNumPhysicalCores() would return 5. Or maybe I'm misunderstanding to which count and index are you referring to?

llvm/lib/Support/Host.cpp
1334

Your example is more clear (not fixed to 0 on one end). If that's what @MaskRay meant, then I agree.

aganea added inline comments.Aug 10 2020, 8:25 AM
llvm/lib/Support/Host.cpp
1334

Sorry wrong example, that should return 4, as CPU 6 & 7 should be on the same core on a SMT system, but you got the idea.

llvm/lib/Support/Host.cpp
1334

@aganea, thanks. This last was something I missed. The "physical" part of the function name is actually significant.

Kai updated this revision to Diff 284629.Aug 11 2020, 3:07 AM
  • Remove list of systems from comment.
Kai marked an inline comment as done.Aug 11 2020, 3:08 AM
This revision was automatically updated to reflect the committed changes.