RFR: 8334078: RISC-V: TestIntVect.java fails after JDK-8332153 when running without RVV

Hamlin Li mli at openjdk.org
Fri Jun 14 17:29:16 UTC 2024


On Fri, 14 Jun 2024 15:54:57 GMT, Gui Cao <gcao at openjdk.org> wrote:

>>> Could there be any naming conflicting in the future? ie. there will be extension named rvd, etc. I'm not sure if the current riscv extension naming convention will avoid this situation, if answer is postive, then it looks good.
>> 
>> Hi, Thanks for the review.
>> As I see it, new riscv extensions are now officially named with a Z prefix, and it's unlikely that rvd will be used for a future extenstion, as it's too ambiguous. Also, I see software like QEMU[1] also uses names like RVI | RVM | RVA | RVF | RVD | RVC elsewhere, so I guess it's not a big problem for us to use rv as a prefix.
>> 
>> [1] https://github.com/qemu/qemu/blob/046a64b9801343e2e89eef10c7a48eec8d8c0d4f/target/riscv/cpu.c#L436-L442
>
>> Another fix (avoid any potential naming conflict in the future) could be use `CPUInfo.getFeatures()` instead of WHITE_BOX.getCPUFeatures() at: https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/ir_framework/test/IREncodingPrinter.java#L419
>> 
>> The existing checking with WHITE_BOX.getCPUFeatures() is error-prone, I suppose it would break this or that in the future.
> 
> I prefer to use rvv for matching in the tests as I think the current approach (vm.cpu.features ~= ".*v,.*") is error-prone because it's easy for people to ignore the comma which is needed for single-character riscv extensions.

Yes, rv* is much better, I'm OK with this renaming.

At the same time, can you fix `WHITE_BOX.getCPUFeatures()` with `CPUInfo.getFeatures()` in IREncodingPrinter.java? As I think it's the final fix for this kind of issue. As I said, with a `String.contains(xxx)`, it could fail with other cpu features in the future, as it mixes all cpu features in one long string, and there is no guarantee the similar issue will not happen again.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19686#discussion_r1640145033


More information about the hotspot-dev mailing list