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)

Reviewers

Fei Yang (@RealFYang - Reviewer)
Feilong Jiang (@feilongjiang - Committer)

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

@Hamlin-Li This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8358892: RISC-V: jvm crash when running dacapo sunflow after JDK-8352504
8359045: RISC-V: construct test to verify invocation of C2_MacroAssembler::enc_cmove_cmp_fp => BoolTest::ge/gt

Co-authored-by: Fei Yang <[email protected]>
Reviewed-by: fyang, fjiang

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 145 new commits pushed to the master branch:

0e725c6: 8354447: Missing test for retroactive @SuppressWarnings("dangling-doc-comments") behavior
b3f5608: 8358094: Test vmTestbase/nsk/jvmti/AttachOnDemand/attach045/TestDescription.java still times out after JDK-8357282
ee35f63: 8359373: Split stubgen initial blob into pre and post-universe blobs
... and 142 more: https://git.openjdk.org/jdk/compare/b6d60280e789436c7f9e3cd1447c8f77b77e77b4...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

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

Hamlin-Li · 2025-06-13T09:58:29Z

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

This failure means it does not go to BoolTest::ge path, but the calculation is still correct.

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

Did you have other vm options when trigger the tests?

I just added more tests, can you try it in your environment? It passed in my local env.

Hamlin-Li · 2025-06-13T10:32:23Z

Maybe we should not assert the log is actually outputted, but just print a message for reference. As what we really want is the correctness of the CMoveI + CmpF/D, but not the specific path it must go through.

~~I'll change assert to a log message in test, to avoid possible false alarms in the future.~~

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp

RealFYang

Latest version looks great. Thanks!
This should also be backported to jdk25 branch: https://github.com/openjdk/jdk/tree/jdk25

feilongjiang

Looks good!

Hamlin-Li · 2025-06-16T10:19:14Z

Thank you @RealFYang @feilongjiang for reviewing.

/integrate

openjdk · 2025-06-16T10:20:28Z

Going to push as commit 9d06057.
Since your change was applied there have been 154 commits pushed to the master branch:

fedd0a0: 8359248: JFR: Help text for-XX:StartFlightRecording:report-on-exit should explain option can be repeated
79497ef: 8348574: Simplify c1/c2_globals inclusions
8416ca3: 8346982: Remove JMX javadoc duplication that was in place due to JDK-6369229
... and 151 more: https://git.openjdk.org/jdk/compare/b6d60280e789436c7f9e3cd1447c8f77b77e77b4...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-06-16T10:20:35Z

@Hamlin-Li Pushed as commit 9d06057.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

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

more tests

962f494

more log

d94c631

hamlin added 2 commits June 13, 2025 10:54

no assert

0f5ca7d

refine comments

e6d172f

remove log

7638e27

RealFYang reviewed Jun 13, 2025

View reviewed changes

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

comments

d3dff79

RealFYang approved these changes Jun 13, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 13, 2025

feilongjiang approved these changes Jun 14, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 16, 2025

openjdk bot closed this Jun 16, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 16, 2025

Hamlin-Li deleted the BoolTest-ge-gt branch June 16, 2025 10:21

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

Uh oh!

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

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Jun 9, 2025

Uh oh!

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

Uh oh!

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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

RealFYang left a comment

Choose a reason for hiding this comment

Uh oh!

feilongjiang left a comment

Choose a reason for hiding this comment

Uh oh!

Hamlin-Li commented Jun 16, 2025

Uh oh!

openjdk bot commented Jun 16, 2025

Uh oh!

openjdk bot commented Jun 16, 2025

Uh oh!

Uh oh!

Hamlin-Li commented Jun 9, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 9, 2025 •

edited

Loading

mlbridge bot commented Jun 9, 2025 •

edited

Loading

RealFYang commented Jun 9, 2025 •

edited

Loading

RealFYang commented Jun 11, 2025 •

edited

Loading

Hamlin-Li commented Jun 13, 2025 •

edited

Loading

Hamlin-Li commented Jun 13, 2025 •

edited

Loading