RFR: 8368950: RISC-V: fail to catch out of order declarations among dependent cpu extensions/flags

[Fei Yang]

On Tue, 30 Sep 2025 11:25:30 GMT, Hamlin Li <mli at openjdk.org> wrote:

> 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/r...

src/hotspot/cpu/riscv/vm_version_riscv.hpp line 73:

> 71:     // For non-ext flags, they don't have dependency relationship among each other,
> 72:     // in this situation, just return the default value -1.
> 73:     virtual int dependent_index() { return -1; }

I wonder if we can make this feature index a bit more general. Currently, we only have `_cpu_feature_index` for subclass RVExtFeatureValue. But I think extensions and non-extensions could both have their own feature index for their group respectively. So maybe we can add a `_feature_index` member here for this base class and a `feature_index()` method, which seems cleaner to me.

src/hotspot/cpu/riscv/vm_version_riscv.hpp line 106:

> 104:     }
> 105: 
> 106:     void verify_deps(RVFeatureValue* dep0, ...) {

Since this method is only for debug purposes, we should put it under `#ifndef PRODUCT`.

src/hotspot/cpu/riscv/vm_version_riscv.hpp line 143:

> 141:   void update_flag() {                                                                                      \
> 142:       assert(enabled(), "Must be.");                                                                        \
> 143:       verify_deps(dep0, ##__VA_ARGS__);                                                                     \

Suggestion: `DEBUG_ONLY(verify_deps(dep0, ##__VA_ARGS__));`

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/27572#discussion_r2428412983
PR Review Comment: https://git.openjdk.org/jdk/pull/27572#discussion_r2428417019
PR Review Comment: https://git.openjdk.org/jdk/pull/27572#discussion_r2428443386