Page MenuHomePhabricator

[SystemZ][z/OS] add code for hardware_concurrency()
AbandonedPublic

Authored by muiez on Sep 19 2022, 1:02 PM.

Details

Reviewers
SeanP
zibi
fanbo-meng
ldionne
Group Reviewers
Restricted Project
Summary

Use the CSD_NUMBER_ONLINE_CPUS entry to return the hardware_concurrency for MVS.

Diff Detail

Event Timeline

muiez created this revision.Sep 19 2022, 1:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2022, 1:02 PM
muiez requested review of this revision.Sep 19 2022, 1:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2022, 1:02 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
zibi accepted this revision.Sep 20 2022, 1:08 PM

LGTM

ldionne added inline comments.Sep 20 2022, 2:42 PM
libcxx/src/thread.cpp
110

There is no named API for this on z/OS?

muiez added inline comments.Sep 23 2022, 7:34 AM
libcxx/src/thread.cpp
110

Unfortunately, there isn't an API for this on z/OS.

muiez updated this revision to Diff 470595.Oct 25 2022, 12:25 PM

Move z/OS hardware_concurrency implementation to support/ibm directory.

zibi added inline comments.Oct 25 2022, 12:53 PM
libcxx/src/support/ibm/hardware_concurrency_zos.cpp
37 ↗(On Diff #470595)

Why don't you rename this to hardware_concurrency_zos() , make it static function and call it from the original place in thread.cpp?

muiez updated this revision to Diff 470628.Oct 25 2022, 3:34 PM
muiez set the repository for this revision to rG LLVM Github Monorepo.

Refactor

zibi added a comment.Oct 28 2022, 6:48 AM

LGTM

libcxx/include/thread
275 ↗(On Diff #470628)

This could be non-member static function in ibm namespace, same as we did nonosleep()..

philnik added inline comments.
libcxx/include/thread
275 ↗(On Diff #470628)

You have to __uglify the name.

zibi added inline comments.Oct 28 2022, 7:04 AM
libcxx/include/thread
275 ↗(On Diff #470628)

Are you refering to hardware_concurrency_zos() or nonosleep()?
Sorry about the typo, it should be nanosleep().

Do you have any suggestion if you refer to hardware_concurrency_zos() ?

ldionne requested changes to this revision.Oct 28 2022, 7:04 AM

I am extremely concerned by the need to introduce these sorts of arcane implementation details in the library for z/OS, especially since we don't have a clear idea of where these patches are going to stop.

libc++ is not a "low level system library". It's a library layered on top of C libraries and other standard (or mostly standard) APIs like POSIX and others. It's fine to have some amount of low level and platform specific stuff, but we're not going to implement from scratch APIs that should be provided by any decent OS.

libcxx/include/thread
274 ↗(On Diff #470628)

This can't have inline visibility if it's in the built library.

275 ↗(On Diff #470628)

And yeah it shouldn't be in the thread class either way.

libcxx/src/support/ibm/hardware_concurrency_zos.cpp
1 ↗(On Diff #470628)

We don't put file names here anymore.

This revision now requires changes to proceed.Oct 28 2022, 7:04 AM
muiez abandoned this revision.EditedOct 28 2022, 8:40 AM
muiez updated this revision to Diff 471569.

I am extremely concerned by the need to introduce these sorts of arcane implementation details in the library for z/OS, especially since we don't have a clear idea of where these patches are going to stop.

libc++ is not a "low level system library". It's a library layered on top of C libraries and other standard (or mostly standard) APIs like POSIX and others. It's fine to have some amount of low level and platform specific stuff, but we're not going to implement from scratch APIs that should be provided by any decent OS.

Thanks for the feedback. I'll close this revision until we have an API for this on z/OS. It should be noted that it is in the works but not feasible in the short term, which is why we upstreamed this workaround in the meantime. To be more upstream friendly, we wanted to keep it in the support/ibm directory until we have an API.