[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.
| #elif define(CPU_FEATURES_ARCH_RISCV) | |
| #elif defined(CPU_FEATURES_ARCH_RISCV) |
There was a problem hiding this comment.
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.
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.
@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.
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.
|
Looks reasonable |
smuzaffar
force-pushed
the
core-riscv-fix1
branch
from
10ee381
to
ec6a7d1
Compare
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-approved
code-checks-approved
labels
|
+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
FWCoreonriscv64architecture. @makortel ,GetRiscvInfo()returns [a] structure. Let me know if we should include something formRiscvFeaturestoo.[a] https://github.com/google/cpu_features/blob/main/include/cpuinfo_riscv.h#L44-L48