8295926: RISC-V: C1: Fix LIRGenerator::do_LibmIntrinsic #10867

zhengxiaolinX · 2022-10-26T04:57:11Z

The ported logic of LIRGenerator::do_LibmIntrinsic has a correctness problem, which will kill argument registers when the current libm intrinsic's operand is also a libm intrinsic, such as:

(dpow val1 (dlog val2))

LIRItem walks operands, so the value.load_item_force(cc->at(0)); should be moved below after the LIRItem, or the result of cc->at(0) would be killed. But we might as well keep aligning AArch64's style to reduce some maintenance work.

Reproducer: (pattern extracted from Renaissance gauss-mix)

public class A {

    static int count = 0;

    public static void print(double var) {
        if (count % 10000 == 0) {
            System.out.println(var);
        }
        count++;
    }

    public static void a(double var1, double var2, double var3) {
        double var4 = Math.pow(var3, Math.log(var1 / var2));
        print(var4);
    }

    public static void main(String[] args) {

        for (int i = 0; i < 50000; i++) {
            double var21 = 2.2250738585072014E-308D;
            double var15 = 1.1102230246251565E-16D;
            double d1 = 2.0D;
            A.a(var21, var15, d1);
        }

    }

}

The right answer is

6.461124611136231E-203
6.461124611136231E-203
6.461124611136231E-203
6.461124611136231E-203
6.461124611136231E-203

The current backend gives

6.461124611136231E-203
NaN
NaN
NaN
NaN

Testing a hotspot tier1~4 on qemu.

Thanks,
Xiaolin

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

Issue

JDK-8295926: RISC-V: C1: Fix LIRGenerator::do_LibmIntrinsic

Reviewers

Yadong Wang (@yadongw - Author) ⚠️ Review applies to 01e54b45
Fei Yang (@RealFYang - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10867

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

Using diff file

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

bridgekeeper · 2022-10-26T04:57:46Z

👋 Welcome back xlinzheng! 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 · 2022-10-26T05:00:15Z

@zhengxiaolinX 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 · 2022-10-26T05:04:54Z

Webrevs

TobiHartmann

Wouldn't it make sense to add the reproducer as a test?

zhengxiaolinX · 2022-10-26T05:49:27Z

Wouldn't it make sense to add the reproducer as a test?

Thank you for the advice and will make one.

zhengxiaolinX · 2022-10-26T07:26:17Z

The test is copied and modified from Roland's TestPow2.java in the same folder.

On x86, aarch64, and riscv with this patch it passes; riscv without this patch shows

interpreter = 2.5355263553695413 c1 = 0.844936682323691 c2 = 2.5355263553695413

and fails.

FYI

python3

>>> math.exp(3.1415926)
23.14069139267437
>>> math.log10(math.exp(3.1415926))
1.3643763305680898
>>> math.log(math.log10(math.exp(3.1415926)))
0.3106974235432832
>>> math.tan(math.log(math.log10(math.exp(3.1415926))))
0.32109666300670675
>>> math.cos(math.tan(math.log(math.log10(math.exp(3.1415926)))))
0.94888987383311
>>> math.sin(math.cos(math.tan(math.log(math.log10(math.exp(3.1415926))))))
0.812769262085064
>>> math.pow(3.1415926, math.sin(math.cos(math.tan(math.log(math.log10(math.exp(3.1415926)))))))
2.5355263553695413

Thanks,
Xiaolin

RealFYang · 2022-10-27T12:33:13Z

test/hotspot/jtreg/compiler/floatingpoint/TestLibmIntrinsics.java

+    }
+
+    static void proofread(double ans) {
+        if (Math.abs(ans - expected) > 1e8) {


If we want to do equivalence check here, shouldn't we compare with 1e-8 instead of 1e8?
That said, do we really need this "proofread" check? I am assuming the check at line #80 is sufficient for this issue.

Sorry for the error - yes, it should have been 1e-8, but somehow the - ran away when it was written :-).
And yes again: only the former check matters, so it is removed.

yadongw

@zhengxiaolinX Nice catch. LGTM.

RealFYang

Updated change looks good. Thanks for finding and fixing this.

openjdk · 2022-10-28T06:26:47Z

@zhengxiaolinX 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:

8295926: RISC-V: C1: Fix LIRGenerator::do_LibmIntrinsic

Reviewed-by: yadongwang, fyang

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 117 new commits pushed to the master branch:

fd668dc: 8295537: Enhance TRACE_METHOD_LINKAGE to show the target MethodHandle
182c215: 8295994: Remove left over InetAddressContainer class
78763fc: 8295000: java/util/Formatter/Basic test cleanup
907d583: 8295323: Unnecessary HashTable usage in StyleSheet
2157145: 8293858: Change PKCS7 code to use default SecureRandom impl instead of SHA1PRNG
b8ad6cd: 8294461: wrong effectively final determination by javac
d667895: 8294399: (ch) Refactor some methods out of sun.nio.ch.UnixFileDispatcherImpl
628820f: 8283093: JMX connections should default to using an ObjectInputFilter
521e712: 8286431: Do not use resource array in posix mmap_attach_shared()
4d9a1cd: 8292159: TYPE_USE annotations on generic type arguments of record components discarded
... and 107 more: https://git.openjdk.org/jdk/compare/f502ab85c987be827d36b0a29f77ec5ce5bb3d01...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann, @RealFYang) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

zhengxiaolinX · 2022-10-28T06:49:57Z

Thank you all for your reviews and comments!

/integrate

openjdk · 2022-10-28T06:51:40Z

@zhengxiaolinX
Your change (at version 9334ce7) is now ready to be sponsored by a Committer.

RealFYang · 2022-10-28T11:55:18Z

/sponsor

openjdk · 2022-10-28T11:56:36Z

Going to push as commit 1fdbb1b.
Since your change was applied there have been 120 commits pushed to the master branch:

cf5546b: 8291336: Add ideal rule to convert floating point multiply by 2 into addition
4b89fce: 8291781: assert(!is_visited) failed: visit only once with -XX:+SuperWordRTDepCheck
d5d3424: 8295405: Add cause in a couple of IllegalArgumentException and InvalidParameterException shown by sun/security/pkcs11 tests
fd668dc: 8295537: Enhance TRACE_METHOD_LINKAGE to show the target MethodHandle
182c215: 8295994: Remove left over InetAddressContainer class
78763fc: 8295000: java/util/Formatter/Basic test cleanup
907d583: 8295323: Unnecessary HashTable usage in StyleSheet
2157145: 8293858: Change PKCS7 code to use default SecureRandom impl instead of SHA1PRNG
b8ad6cd: 8294461: wrong effectively final determination by javac
d667895: 8294399: (ch) Refactor some methods out of sun.nio.ch.UnixFileDispatcherImpl
... and 110 more: https://git.openjdk.org/jdk/compare/f502ab85c987be827d36b0a29f77ec5ce5bb3d01...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-10-28T11:56:51Z

@RealFYang @zhengxiaolinX Pushed as commit 1fdbb1b.

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

Fix simply

6706d1a

openjdk bot added the rfr Pull request is ready for review label Oct 26, 2022

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 26, 2022

TobiHartmann reviewed Oct 26, 2022

View reviewed changes

Add one test

e686497

Maybe a license

01e54b4

RealFYang reviewed Oct 27, 2022

View reviewed changes

yadongw approved these changes Oct 28, 2022

View reviewed changes

Fix the newly-added test

9334ce7

RealFYang approved these changes Oct 28, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 28, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 28, 2022

openjdk bot added the integrated Pull request has been integrated label Oct 28, 2022

openjdk bot closed this Oct 28, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8295926: RISC-V: C1: Fix LIRGenerator::do_LibmIntrinsic #10867

8295926: RISC-V: C1: Fix LIRGenerator::do_LibmIntrinsic #10867

zhengxiaolinX commented Oct 26, 2022 •

edited

bridgekeeper bot commented Oct 26, 2022

openjdk bot commented Oct 26, 2022

mlbridge bot commented Oct 26, 2022 •

edited

TobiHartmann left a comment

zhengxiaolinX commented Oct 26, 2022

zhengxiaolinX commented Oct 26, 2022 •

edited

RealFYang Oct 27, 2022

zhengxiaolinX Oct 28, 2022

yadongw left a comment

RealFYang left a comment

openjdk bot commented Oct 28, 2022

zhengxiaolinX commented Oct 28, 2022

openjdk bot commented Oct 28, 2022

RealFYang commented Oct 28, 2022

openjdk bot commented Oct 28, 2022

openjdk bot commented Oct 28, 2022

Navigation Menu

8295926: RISC-V: C1: Fix LIRGenerator::do_LibmIntrinsic #10867

8295926: RISC-V: C1: Fix LIRGenerator::do_LibmIntrinsic #10867

Conversation

zhengxiaolinX commented Oct 26, 2022 • edited

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 26, 2022

openjdk bot commented Oct 26, 2022

mlbridge bot commented Oct 26, 2022 • edited

Webrevs

TobiHartmann left a comment

Choose a reason for hiding this comment

zhengxiaolinX commented Oct 26, 2022

zhengxiaolinX commented Oct 26, 2022 • edited

RealFYang Oct 27, 2022

Choose a reason for hiding this comment

zhengxiaolinX Oct 28, 2022

Choose a reason for hiding this comment

yadongw left a comment

Choose a reason for hiding this comment

RealFYang left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 28, 2022

zhengxiaolinX commented Oct 28, 2022

openjdk bot commented Oct 28, 2022

RealFYang commented Oct 28, 2022

openjdk bot commented Oct 28, 2022

openjdk bot commented Oct 28, 2022

zhengxiaolinX commented Oct 26, 2022 •

edited

mlbridge bot commented Oct 26, 2022 •

edited

zhengxiaolinX commented Oct 26, 2022 •

edited