-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CORE] [RISCV] Fix needed for riscv64 #45574
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45574/41071 |
A new Pull Request was created by @smuzaffar for master. It involves the following packages:
@Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
FWCore/Services/plugins/CPU.cc
Outdated
@@ -28,6 +28,8 @@ | |||
#include "cpu_features/cpuinfo_aarch64.h" | |||
#elif defined(CPU_FEATURES_ARCH_PPC) | |||
#include "cpu_features/cpuinfo_ppc.h" | |||
#elif define(CPU_FEATURES_ARCH_RISCV) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#elif define(CPU_FEATURES_ARCH_RISCV) | |
#elif defined(CPU_FEATURES_ARCH_RISCV) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah thanks @fwyzard , fixed now
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45574/41076 |
Pull request #45574 was updated. @Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please check and sign again. |
please test |
+1 Size: This PR adds an extra 24KB to repository Comparison SummarySummary:
|
@dan131riley please take a look |
I think this is a reasonable start, we can always expand later if something additional seems useful. |
@@ -76,6 +76,8 @@ namespace edm { | |||
__asm__ __volatile__("isb; mrs %0, cntvct_el0" : "=r"(ret)); | |||
return ret; | |||
} | |||
#elif defined(__riscv) | |||
static __inline__ unsigned long long rdtsc(void) { return 0; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have a comment here (or a #warning
as for ARMv7 above) explaining why this function returns 0 for RISC-V.
(in any case this support does not seem super important in the near or medium term, given that it seems to be used only in handful of unit tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@makortel , my bad, I should not have included it here. Looking at https://gist.github.com/XieJiSS/18acd12685ecc0913d43d6bcfb3baefd , I see that there is way to get this info for riscv. You think something like the following should be enough for riscv ?
#elif defined(__riscv) && __riscv_xlen == 64
static __inline__ unsigned long long rdtsc(void)
uint64_t cycles;
asm volatile("rdcycle %0" : "=r"(cycles));
return cycles;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You think something like the following should be enough for riscv ?
#elif defined(__riscv) && __riscv_xlen == 64 static __inline__ unsigned long long rdtsc(void) uint64_t cycles; asm volatile("rdcycle %0" : "=r"(cycles)); return cycles; }
I'm by far not an expert of RISC-V assembly, but based on quick searches it looks a reasonable starting point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Looks reasonable |
10ee381
to
ec6a7d1
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45574/41093 |
Pull request #45574 was updated. @Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please check and sign again. |
please test |
+1 Size: This PR adds an extra 20KB to repository Comparison SummarySummary:
|
Comparison differences are related to #45505 |
+core |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @antoniovilela, @sextonkennedy, @mandrenguyen, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
These changes are needed to build
FWCore
onriscv64
architecture. @makortel ,GetRiscvInfo()
returns [a] structure. Let me know if we should include something formRiscvFeatures
too.[a] https://github.com/google/cpu_features/blob/main/include/cpuinfo_riscv.h#L44-L48