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

[Fei Yang]

On Tue, 14 Oct 2025 15:22:05 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)
>> 
>> 
>> ...
>
> Hamlin Li has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 19 additional commits since the last revision:
> 
>  - reorder
>  - Merge branch 'master' into fix-out-of-order-declarations
>  - Merge branch 'openjdk:master' into master
>  - Merge branch 'openjdk:master' into master
>  - Merge branch 'openjdk:master' into master
>  - rename
>  - move dependency checks into RVExtFeatureValue
>  - only verify_deps when debug
>  - initial commit
>  - initial commit
>  - ... and 9 more: https://git.openjdk.org/jdk/compare/492b4569...d8f39f70

Latest version LGTM. Thanks.

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

> 173:     }
> 174: 
> 175: #ifndef PRODUCT

Nit: can we switch to use `#ifdef ASSERT` instead in order to better match `DEBUG_ONLY` at the use site?


 #ifdef ASSERT
 #define DEBUG_ONLY(code) code
 #define NOT_DEBUG(code)
 #define NOT_DEBUG_RETURN  /*next token must be ;*/
 #else // ASSERT
 #define DEBUG_ONLY(code)
 #define NOT_DEBUG(code) code
 #define NOT_DEBUG_RETURN {}
 #endif // ASSERT

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

Marked as reviewed by fyang (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27572#pullrequestreview-3338128745
PR Review Comment: https://git.openjdk.org/jdk/pull/27572#discussion_r2430939120