On Mon, 7 Apr 2025 14:23:52 GMT, Hamlin Li <m...@openjdk.org> wrote: > Hi, > Can you help to review this patch? > On riscv, CMoveI/L already were implemented, but there are some gap: > 1. CMoveI/L does not support comparison with float/double, corresponding > tests are not turn on either. > 2. Some optimization of C2 is not turned on, e.g. `Phi -> CMove -> min_max`. > 3. lack of some corresponding performance tests. > > Also there are some issue with current Zicond: > 1. UseZicond is turned on automatically by hwprobe, but jmh tests show that > it's not always bring benefit, in some situation it even brings regression, > the reason is the generated code by Zicond is much larger than branch > version, in particular when it's in a loop and unrolled. > > This patch on riscv is to: > 1. add CMoveI/L comparing float/double, and corresponding tests, > 2. enable more C2 optimization, > 3. add more benchmark tests, > 4. turn off UseZicond by default. > > Thanks!
Hi, @Hamlin-Li , Thanks for looking at this part. I once created JBS https://bugs.openjdk.org/browse/JDK-8346786 about `ConditionalMoveLimit` on RISC-V. I have two questions after a cursory look. src/hotspot/cpu/riscv/vm_version_riscv.cpp line 257: > 255: if (ConditionalMoveLimit > 0) { > 256: FLAG_SET_DEFAULT(ConditionalMoveLimit, 0); > 257: } Maybe we should check `UseZicond` and only enable `UseCMoveUnconditionally` & `ConditionalMoveLimit` conditionally? I don't see how enabling CMove will bring us performance benefit without `Zicond`. It's done with conditional branches in CPU backend. src/hotspot/cpu/riscv/vm_version_riscv.cpp line 461: > 459: FLAG_SET_DEFAULT(UseZicond, false); > 460: warning("UseZicond is turned off automatically. Turn it on with > -XX:+UseZicond explicitly."); > 461: } Does this mean `UseZicond` could not be enabled on the command line? And I witnessed quite some warning when doing a native build. If `UseZicond` causes regression for some cases, is it more reasonable to not auto-enable it through hwprobe [1]? Or only enable it for debug builds like https://github.com/openjdk/jdk/pull/24478 does? [1] https://github.com/openjdk/jdk/blob/master/src/hotspot/os_cpu/linux_riscv/riscv_hwprobe.cpp#L228 ------------- PR Review: https://git.openjdk.org/jdk/pull/24490#pullrequestreview-2748525865 PR Review Comment: https://git.openjdk.org/jdk/pull/24490#discussion_r2032530242 PR Review Comment: https://git.openjdk.org/jdk/pull/24490#discussion_r2032292830
