RFR: 8369685: RISC-V: refactor code related to RVFeatureValue::enabled

[Fei Yang]

On Mon, 13 Oct 2025 13:02:50 GMT, Hamlin Li <mli at openjdk.org> wrote:

> Hi,
> Can you help to review this patch?
> @RealFYang 
> 
> Original discussion is here: https://github.com/openjdk/jdk/pull/27562#discussion_r2415984981.
> Although seems we can not remove it, but we can still remove some of the redundant check like `mvendorid.enabled()`.
> And also do some simple refactoring related to enable/disable/enabled and so on.
> 
> Thanks

src/hotspot/cpu/riscv/vm_version_riscv.cpp line 201:

> 199:   if (UseZicboz && zicboz_block_size.enabled()) {
> 200:     assert(is_power_of_2(zicboz_block_size.value()) &&
> 201:            zicboz_block_size.value() > 0, "Sanity");

Note the previous discussion about why we need this `> 0` check [1].
@robehn mentions that the kernel seems to have a bug: if it can't retrive those value it will return 0 as value for those.

So maybe simply do:

if (UseZicboz && zicboz_block_size.value() > 0) {
  assert(is_power_of_2(zicboz_block_size.value()), "Sanity");
  ......


[1] https://github.com/openjdk/jdk/pull/27155#issuecomment-3270608461

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

> 158:       RVExtFeatures::current()->clear_feature(_cpu_feature_index);
> 159:     }
> 160:     int64_t value() { return enabled(); }

Seems redundant in functionality with `enabled()` for extension features. I think this `value()` method should only apply to non-extension features.

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

> 163:   class RVNonExtFeatureValue : public RVFeatureValue {
> 164:     int64_t _value;
> 165:     static const int64_t DISABLED_VALUE = -1;

I am not sure but is it better to rename it as `DEFAULT_VALUE`?

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

> 169:       _value(DISABLED_VALUE) {
> 170:     }
> 171:     bool enabled() { return _value != DISABLED_VALUE; }

Do we really need this `enabled()` method for non-extension features? I think this `enabled()` method should only apply to extension features.

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

> 170:     }
> 171:     bool enabled() { return _value != DISABLED_VALUE; }
> 172:     void enable_feature(int64_t value) { _value = value; }

Can you add an assertion to make sure that the passed `value` won't equal to the default value -1?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27771#discussion_r2430978496
PR Review Comment: https://git.openjdk.org/jdk/pull/27771#discussion_r2430990502
PR Review Comment: https://git.openjdk.org/jdk/pull/27771#discussion_r2430979297
PR Review Comment: https://git.openjdk.org/jdk/pull/27771#discussion_r2430987912
PR Review Comment: https://git.openjdk.org/jdk/pull/27771#discussion_r2430981579