8358892: RISC-V: jvm crash when running dacapo sunflow after JDK-8352504 #25696

Hamlin-Li · 2025-06-09T16:27:54Z

Hi,
Can you help to review this patch?

Thanks!

Currently, this issue is only reproducible with Dacapo sunflow.
I tried to construct a simpler jtreg test to reproduce the issue, but can not find a way to do it till now, this task is tracked by https://bugs.openjdk.org/browse/JDK-8359045.

So, currently I can only verify the code by reviewing it.
Or maybe it's better to leave it until we find the test?

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issues

JDK-8358892: RISC-V: jvm crash when running dacapo sunflow after JDK-8352504 (Bug - P3)(⚠️ The fixVersion in this issue is [25] but the fixVersion in .jcheck/conf is 26, a new backport will be created when this pr is integrated.)
JDK-8359045: RISC-V: construct test to verify invocation of C2_MacroAssembler::enc_cmove_cmp_fp => BoolTest::ge/gt (Enhancement - P4)

Contributors

Fei Yang <[email protected]>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25696/head:pull/25696
$ git checkout pull/25696

Update a local copy of the PR:
$ git checkout pull/25696
$ git pull https://git.openjdk.org/jdk.git pull/25696/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25696

View PR using the GUI difftool:
$ git pr show -t 25696

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25696.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-09T16:28:42Z

👋 Welcome back mli! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-06-09T16:30:03Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-06-09T16:30:47Z

@Hamlin-Li The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-06-09T16:36:28Z

Webrevs

RealFYang · 2025-06-09T22:20:57Z

Try this:

public class Test {

    public static int[] nl = { 0, 0, 0 };
    public static int[] nr = { 1, 1, 1 };

    public static float[] nodeMin = { 0.0f, 0.0f, 0.0f };
    public static float[] nodeMax = { 0.1f, 0.1f, 0.1f };

    // for case BoolTest::ge
    public static int test1(int axis, int pPlanar, float dl, float dr) {
        boolean planarLeft = !(dl <= dr);
        int numLeft = nl[axis] + (planarLeft ? pPlanar : 0);
        int numRight = nr[axis] + (planarLeft ? 0 : pPlanar);
        return numLeft + numRight;
    }

    // for case BoolTest::gt
    public static int test2(int axis, int pPlanar, float dl, float dr) {
        boolean planarLeft = !(dl < dr);
        int numLeft = nl[axis] + (planarLeft ? pPlanar : 0);
        int numRight = nr[axis] + (planarLeft ? 0 : pPlanar);
        return numLeft + numRight;
    }

    public static void main(String[] args) {
        int ret = 0;

        // test case BoolTest::ge
        for (int i = 0; i < 20000; i++) {
            if (i % 2 == 0) {
              ret = test1(i % 3, i % 10, nodeMin[i % 3], nodeMax[i % 3]);
            } else {
              ret = test1(i % 3, i % 10, nodeMax[i % 3], nodeMin[i % 3]);
            }
        }
        System.out.println("test1 passed. result = " + ret);

        // test case BoolTest::gt
        for (int i = 0; i < 20000; i++) {
            if (i % 2 == 0) {
              ret = test2(i % 3, i % 10, nodeMin[i % 3], nodeMax[i % 3]);
            } else {
              ret = test2(i % 3, i % 10, nodeMax[i % 3], nodeMin[i % 3]);
            }
        }
        System.out.println("test2 passed. result = " + ret);
    }
}

$ java -XX:-TieredCompilation Test

This reduced test can reproduce the crash and cover both BoolTest::gt and BoolTest::ge cases.

Edit: Here is a more simplified version of the test.

public class Test {

    // return 1 if dl > dr, 0 otherwise.
    public static int test_float_gt(float dl, float dr) {
        return !(dl <= dr) ? 1 : 0;
    }

    // return 1 if dl <= dr, 0 otherwise.
    public static int test_float_ge(float dl, float dr) {
        return !(dl < dr) ? 1 : 0;
    }

    public static void main(String[] args) throws Exception {
        int ret = 0;

        // test case BoolTest::gt
        for (int i = 0; i < 20000; i++) {
            if ((i % 2) == 0) {
                ret = test_float_gt(1.0f, 2.0f);
                if (ret != 0) {
                    throw new Exception("test_float_gt failed.");
                }
            } else {
                ret = test_float_gt(2.0f, 1.0f);
                if (ret != 1) {
                    throw new Exception("test_float_gt failed.");
                }
            }
        }
        System.out.println("test_float_gt passed.");

        // test case BoolTest::ge
        for (int i = 0; i < 20000; i++) {
            if ((i % 2) == 0) {
                ret = test_float_ge(1.0f, 2.0f);
                if (ret != 0) {
                    throw new Exception("test_float_ge failed.");
                }
            } else {
                ret = test_float_ge(2.0f, 1.0f);
                if (ret != 1) {
                    throw new Exception("test_float_ge failed.");
                }
            }
        }
        System.out.println("test_float_ge passed.");
    }
}

Hamlin-Li · 2025-06-10T11:47:40Z

/solves JDK-8359045

/contributor add @RealFYang

openjdk · 2025-06-10T11:48:02Z

@Hamlin-Li
Adding additional issue to solves list: 8359045: RISC-V: construct test to verify invocation of C2_MacroAssembler::enc_cmove_cmp_fp => BoolTest::ge/gt.

openjdk · 2025-06-10T11:48:27Z

@Hamlin-Li
Contributor Fei Yang <[email protected]> successfully added.

Hamlin-Li · 2025-06-10T16:07:08Z

We need new implementation for BoolTest::ge/gt, because of NaN cases. It's done.
Also add tests based on @RealFYang 's example. Thank you @RealFYang !

RealFYang · 2025-06-11T03:37:36Z

Hi,

We need new implementation for BoolTest::ge/gt, because of NaN cases. It's done.

Hmm, I don't understand why your first commit (e5b06b5) won't work for NaN cases.
Do you have more details or maybe a small test case to demo your concern?

I also changed my test a bit trying NaN cases and it still works if I use your first commit (e5b06b5).

public class Test {

    // return 1 if dl > dr, 0 otherwise.
    public static int test_float_gt(float dl, float dr) {
        return !(dl <= dr) ? 1 : 0;
    }

    // return 1 if dl <= dr, 0 otherwise.
    public static int test_float_ge(float dl, float dr) {
        return !(dl < dr) ? 1 : 0;
    }

    public static void main(String[] args) throws Exception {
        int ret = 0;

        // test case BoolTest::ge
        for (int i = 0; i < 20000; i++) {
            if ((i % 2) == 0) {
                ret = test_float_gt(1.0f, Float.NaN);                  <===============
                if (ret != 1) {
                    throw new Exception("test_float_gt failed.");
                }
            } else {
                ret = test_float_gt(2.0f, 1.0f);
                if (ret != 1) {
                    throw new Exception("test_float_gt failed.");
                }
            }
        }
        System.out.println("test_float_gt passed.");

        // test case BoolTest::gt
        for (int i = 0; i < 20000; i++) {
            if ((i % 2) == 0) {
                ret = test_float_ge(1.0f, Float.NaN);                  <===============
                if (ret != 1) {
                    throw new Exception("test_float_ge failed.");
                }
            } else {
                ret = test_float_ge(2.0f, 1.0f);
                if (ret != 1) {
                    throw new Exception("test_float_ge failed.");
                }
            }
        }
        System.out.println("test_float_ge passed.");
    }
}

Hamlin-Li · 2025-06-11T08:32:10Z

Hi,

We need new implementation for BoolTest::ge/gt, because of NaN cases. It's done.

Hmm, I don't understand why your first commit (e5b06b5) won't work for NaN cases.

I think the reason is the original assumption is not right about the behaviour of cmov_cmp_fp_ge/gt.

Do you have more details or maybe a small test case to demo your concern?

You can see failures when running new tests if revert back to first commit of implementation.

test/hotspot/jtreg/compiler/c2/irTests/TestFPComparison2.java

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp

RealFYang · 2025-06-13T06:52:35Z

Hi, I changed your test a bit and I see test failure on my linux-riscv64 platform (no Zicond).

diff --git a/test/hotspot/jtreg/compiler/c2/irTests/TestFPComparison2.java b/test/hotspot/jtreg/compiler/c2/irTests/TestFPComparison2.java
index 472c38e009a..77398e1d88b 100644
--- a/test/hotspot/jtreg/compiler/c2/irTests/TestFPComparison2.java
+++ b/test/hotspot/jtreg/compiler/c2/irTests/TestFPComparison2.java
@@ -98,11 +98,11 @@ public static int test_float_BoolTest_ge(float x, float y) {
         //      when neither is NaN, and x > y
         // return 0
         //      when neither is NaN, and x <= y
-        return !(x <= y) ? 1 : 0;
+        return !(x <= y) ? 10 : 20;
     }
     @DontCompile
     public static int golden_float_BoolTest_ge(float x, float y) {
-        return !(x <= y) ? 1 : 0;
+        return !(x <= y) ? 10 : 20;
     }

     @Test
@@ -113,11 +113,11 @@ public static int test_double_BoolTest_ge(double x, double y) {
         //      when neither is NaN, and x > y
         // return 0
         //      when neither is NaN, and x <= y
-        return !(x <= y) ? 1 : 0;
+        return !(x <= y) ? 10 : 20;
     }
     @DontCompile
     public static int golden_double_BoolTest_ge(double x, double y) {
-        return !(x <= y) ? 1 : 0;
+        return !(x <= y) ? 10 : 20;
     }

     @Run(test = {"test_float_BoolTest_ge", "test_double_BoolTest_ge"})

$ make test TEST="test/hotspot/jtreg/compiler/c2/irTests/TestFPComparison2.java" JTREG="TIMEOUT_FACTOR=8"

STDERR:
java.lang.RuntimeException: Not trigger BoolTest::ge: expected true, was false
        at jdk.test.lib.Asserts.fail(Asserts.java:715)
        at jdk.test.lib.Asserts.assertTrue(Asserts.java:545)
        at compiler.c2.irTests.TestFPComparison2.main(TestFPComparison2.java:71)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
        at java.base/java.lang.reflect.Method.invoke(Method.java:565)
        at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:335)
        at java.base/java.lang.Thread.run(Thread.java:1474)

JavaTest Message: Test threw exception: java.lang.RuntimeException
JavaTest Message: shutting down test

initial commit

e5b06b5

openjdk bot added the rfr Pull request is ready for review label Jun 9, 2025

openjdk bot added the hotspot-compiler [email protected] label Jun 9, 2025

hamlin added 2 commits June 10, 2025 16:04

fix BoolTest::ge/gt

2d714f8

tests

45dc494

typo

7da631b

hamlin added 3 commits June 11, 2025 09:05

debug

73e8120

golden values

423cf1c

adjust arguments orders

9848cc2

RealFYang reviewed Jun 12, 2025

View reviewed changes

test/hotspot/jtreg/compiler/c2/irTests/TestFPComparison2.java Outdated Show resolved Hide resolved

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp Outdated Show resolved Hide resolved

hamlin added 2 commits June 12, 2025 16:47

comments

852ef94

review Zicond

eb38253

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8358892: RISC-V: jvm crash when running dacapo sunflow after JDK-8352504 #25696

8358892: RISC-V: jvm crash when running dacapo sunflow after JDK-8352504 #25696

Hamlin-Li commented Jun 9, 2025 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Jun 9, 2025

Uh oh!

openjdk bot commented Jun 9, 2025

Uh oh!

openjdk bot commented Jun 9, 2025

Uh oh!

mlbridge bot commented Jun 9, 2025 •

edited

Loading

Uh oh!

RealFYang commented Jun 9, 2025 •

edited

Loading

Uh oh!

Hamlin-Li commented Jun 10, 2025

Uh oh!

openjdk bot commented Jun 10, 2025

Uh oh!

openjdk bot commented Jun 10, 2025

Uh oh!

Hamlin-Li commented Jun 10, 2025

Uh oh!

RealFYang commented Jun 11, 2025 •

edited

Loading

Uh oh!

Hamlin-Li commented Jun 11, 2025

Uh oh!

Uh oh!

Uh oh!

RealFYang commented Jun 13, 2025

Uh oh!

Uh oh!

8358892: RISC-V: jvm crash when running dacapo sunflow after JDK-8352504 #25696

Are you sure you want to change the base?

8358892: RISC-V: jvm crash when running dacapo sunflow after JDK-8352504 #25696

Conversation

Hamlin-Li commented Jun 9, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Jun 9, 2025

Uh oh!

openjdk bot commented Jun 9, 2025

Uh oh!

openjdk bot commented Jun 9, 2025

Uh oh!

mlbridge bot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

RealFYang commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hamlin-Li commented Jun 10, 2025

Uh oh!

openjdk bot commented Jun 10, 2025

Uh oh!

openjdk bot commented Jun 10, 2025

Uh oh!

Hamlin-Li commented Jun 10, 2025

Uh oh!

RealFYang commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hamlin-Li commented Jun 11, 2025

Uh oh!

Uh oh!

Uh oh!

RealFYang commented Jun 13, 2025

Uh oh!

Uh oh!

Hamlin-Li commented Jun 9, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 9, 2025 •

edited

Loading

RealFYang commented Jun 9, 2025 •

edited

Loading

RealFYang commented Jun 11, 2025 •

edited

Loading