Add support for s390x cpu frequency #728
Conversation
| @@ -81,7 +81,7 @@ | |||
| // This macro can be used to test if UnscaledCycleClock::Frequency() | |||
| // is NominalCPUFrequency() on a particular platform. | |||
| #if (defined(__i386__) || defined(__x86_64__) || \ | |||
| defined(_M_IX86) || defined(_M_X64)) | |||
| defined(_M_IX86) || defined(_M_X64)) || defined(__s390x__) | |||
There was a problem hiding this comment.
https://groups.google.com/forum/#!topic/bit.listserv.ibm-main/mYYcbXg0lGY makes me think that the counter retreived by the stck instruction is a clock, but not one that runs at the CPU frequency, since it seems constant across chips. Can you point me to documentation that shows otherwise?
There was a problem hiding this comment.
Thanks for the notes. Honestly speaking, I'm not very familiar with instruction set or cpu counter, so I guess what you are suggesting is that stck does not return the correct cpu time so the frequency calculated based on this will not be accurate, am I understanding it correctly? If this is the case, could you please give me a hint of what counter do we need to retrieve here so that I could look into it further? Thank you.
|
I am also not sure how likely this API will be used by s390x users, but the reason I would like to fix this is that Tensorflow uses this API in its code, see here, and that also leads to some test case failures with Tensorflow. |
|
TensorFlow should not be doing that. From https://abseil.io/about/compatibility:
|
|
Thanks for pointing out that, I did feel a little bit strange when seeing them using this API rather than another Tensorflow-native API to retrieve CPU frequency. I may raise an issue in the Tensorflow community about this. As for the fix itself, I think I will still look into it a little bit to see if I could fix it. If it is too much trouble, I guess I will then close this PR since it is not a critical bug as you mentioned. Will that be okay if I leave it open now? |
|
Do we need to keep this PR in open state? |
|
Hi, I have raised the issue with Tensorflow and they have not responded yet, so I will follow up on it. Currently, I think it may not worth investing more time in fixing the cpu frequency issue since it is not actually used (or not supposed to be used) anywhere, so I will close this PR for now and I will consider reopening it if things have changed. Thank you. |
As could be seen from this issue, the unit test
SysinfoTest.NominalCPUFrequencyhas always been failing on s390x machines. The reason for the failure is because there is no mechanism in abseil code to retrieve CPU time and frequency for s390x machines. I am adding the methods for s390x by referencing a similar method committed to another project, see here.