RFR: 8368950: RISC-V: fail to catch out of order declarations among dependent cpu extensions/flags
Hamlin Li
mli at openjdk.org
Tue Sep 30 11:34:14 UTC 2025
Hi,
Can you help to review this patch?
## Background
In https://bugs.openjdk.org/browse/JDK-8352218, we introduced dependency among CPU extensions/flags. And to make sure it works, A needs to be declared before B in RV_FEATURE_FLAGS if B depends on A, for example, A is v, B is Zvfh. So in the pr an assert was introduced to make sure A is before B in RV_FEATURE_FLAGS.
But the assert does not work as expected, means if I move A below B in RV_FEATURE_FLAGS (check below for a simple patch, it's based on 7853415217cc17179abf2e160ca735c936017f4e), it can not be caught by the assert.
diff --git a/src/hotspot/cpu/riscv/vm_version_riscv.hpp b/src/hotspot/cpu/riscv/vm_version_riscv.hpp
index 4214d6c53dc..80896f8fffc 100644
--- a/src/hotspot/cpu/riscv/vm_version_riscv.hpp
+++ b/src/hotspot/cpu/riscv/vm_version_riscv.hpp
@@ -169,7 +169,6 @@ class VM_Version : public Abstract_VM_Version {
decl(ext_C , "c" , ('C' - 'A'), true , UPDATE_DEFAULT(UseRVC)) \
decl(ext_Q , "q" , ('Q' - 'A'), true , NO_UPDATE_DEFAULT) \
decl(ext_H , "h" , ('H' - 'A'), true , NO_UPDATE_DEFAULT) \
- decl(ext_V , "v" , ('V' - 'A'), true , UPDATE_DEFAULT(UseRVV)) \
decl(ext_Zicbom , "Zicbom" , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT(UseZicbom)) \
decl(ext_Zicboz , "Zicboz" , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT(UseZicboz)) \
decl(ext_Zicbop , "Zicbop" , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT(UseZicbop)) \
@@ -193,6 +192,7 @@ class VM_Version : public Abstract_VM_Version {
decl(ext_Zvbc , "Zvbc" , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT_DEP(UseZvbc, ext_V)) \
decl(ext_Zvfh , "Zvfh" , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT_DEP(UseZvfh, ext_V)) \
decl(ext_Zvkn , "Zvkn" , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT_DEP(UseZvkn, ext_V)) \
+ decl(ext_V , "v" , ('V' - 'A'), true , UPDATE_DEFAULT(UseRVV)) \
decl(ext_Zicond , "Zicond" , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT(UseZicond)) \
decl(mvendorid , "VendorId" , RV_NO_FLAG_BIT, false, NO_UPDATE_DEFAULT) \
decl(marchid , "ArchId" , RV_NO_FLAG_BIT, false, NO_UPDATE_DEFAULT) \
If added following patch based on the above patch, we can find out what's going on, it will trigger the assert:
# Internal Error (/home/hamlin/workspace/repos/github/jdk-master-tmp-2/src/hotspot/cpu/riscv/vm_version_riscv.hpp:211), pid=430595, tid=430603
# assert((uintptr_t)(&ext_V) > (uintptr_t)(this)) failed: Invalid: dep: 140361453683696 (v), this: 140361453685240 (Zvbc)
diff --git a/src/hotspot/cpu/riscv/vm_version_riscv.hpp b/src/hotspot/cpu/riscv/vm_version_riscv.hpp
index 4214d6c53dc..0ba636dce96 100644
--- a/src/hotspot/cpu/riscv/vm_version_riscv.hpp
+++ b/src/hotspot/cpu/riscv/vm_version_riscv.hpp
@@ -90,8 +90,8 @@ class VM_Version : public Abstract_VM_Version {
void update_flag() { \
assert(enabled(), "Must be."); \
/* dep must be declared before */ \
- assert((uintptr_t)(this) > \
- (uintptr_t)(&dep), "Invalid");\
+ assert((uintptr_t)(&dep) > \
+ (uintptr_t)(this), "Invalid: dep: %ld (%s), this: %ld (%s)", p2i(&dep), dep.pretty(), p2i(this), pretty()); \
if (FLAG_IS_DEFAULT(flag)) { \
if (dep.enabled()) { \
FLAG_SET_DEFAULT(flag, true); \
But I don't think we can rely on the address of different ext_x to determine their declaration order in RV_FEATURE_FLAGS, even if the patch above can catch it.
## Fix
This patch reuses RVExtFeatureValue::_cpu_feature_index as dependent_index to check the declaration order is as expected.
## Test
With the following patch, it will trigger the assert:
# Internal Error (/home/hamlin/workspace/repos/github/jdk-master-aot/src/hotspot/cpu/riscv/vm_version_riscv.hpp:120), pid=708174, tid=708177
# assert(dependent_index() > next->dependent_index()) failed: Invalid
diff --git a/src/hotspot/cpu/riscv/vm_version_riscv.hpp b/src/hotspot/cpu/riscv/vm_version_riscv.hpp
index ae29bd36a10..8e602fb2413 100644
--- a/src/hotspot/cpu/riscv/vm_version_riscv.hpp
+++ b/src/hotspot/cpu/riscv/vm_version_riscv.hpp
@@ -277,7 +277,6 @@ class VM_Version : public Abstract_VM_Version {
decl(ext_C , c , ('C' - 'A'), true , UPDATE_DEFAULT(UseRVC)) \
decl(ext_Q , q , ('Q' - 'A'), true , NO_UPDATE_DEFAULT) \
decl(ext_H , h , ('H' - 'A'), true , NO_UPDATE_DEFAULT) \
- decl(ext_V , v , ('V' - 'A'), true , UPDATE_DEFAULT(UseRVV)) \
decl(ext_Zicbom , Zicbom , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT(UseZicbom)) \
decl(ext_Zicboz , Zicboz , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT(UseZicboz)) \
decl(ext_Zicbop , Zicbop , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT(UseZicbop)) \
@@ -301,6 +300,7 @@ class VM_Version : public Abstract_VM_Version {
decl(ext_Zvbc , Zvbc , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT_DEP(UseZvbc, &ext_V, nullptr)) \
decl(ext_Zvfh , Zvfh , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT_DEP(UseZvfh, &ext_V, &ext_Zfh, nullptr)) \
decl(ext_Zvkn , Zvkn , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT_DEP(UseZvkn, &ext_V, nullptr)) \
+ decl(ext_V , v , ('V' - 'A'), true , UPDATE_DEFAULT(UseRVV)) \
decl(ext_Zicond , Zicond , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT(UseZicond)) \
Thanks!
-------------
Commit messages:
- initial commit
- initial commit
- Merge branch 'openjdk:master' into master
- Merge branch 'openjdk:master' into master
- Merge branch 'openjdk:master' into master
- Merge branch 'openjdk:master' into master
- Merge branch 'openjdk:master' into master
- Merge branch 'openjdk:master' into master
- Merge branch 'openjdk:master' into master
- Merge branch 'openjdk:master' into master
- ... and 1 more: https://git.openjdk.org/jdk/compare/c0a4c0ba...3205c38c
Changes: https://git.openjdk.org/jdk/pull/27572/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=27572&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8368950
Stats: 34 lines in 1 file changed: 29 ins; 0 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/27572.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/27572/head:pull/27572
PR: https://git.openjdk.org/jdk/pull/27572
More information about the hotspot-dev
mailing list