JDK-8210858: AArch64: remove Math.log intrinsic

[tobiasholenstein]

JDK-8215133 disabled vmIntrinsics::_dlog. Remove it now

Why remove

That Java specification says:

"The computed result must be within 1 ulp of the exact result. Results must be semi-monotonic... whenever the mathematical function is non-decreasing, so is the floating-point approximation, likewise, whenever the mathematical function is non-increasing, so is the floating-point approximation"

There is no proof of the monotonicity of this intrinsics at the moment.

---

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-8210858: AArch64: remove Math.log intrinsic (Enhancement - P4)

Reviewers

• Nick Gasson (@nick-arm - Reviewer) ⚠️ Review applies to f1eaee30
• Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to f1eaee30

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17480

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back tholenstein! 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]

@tobiasholenstein 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]

Webrevs

• 02: Full - Incremental (e6dc5b93)
• 01: Full - Incremental (f1eaee30)
• 00: Full (a0240c4d)

[nick-arm]

Should we also remove MacroAssembler::fast_log() and generate_dlog() as they are unused now?

[tobiasholenstein]

Should we also remove MacroAssembler::fast_log() and generate_dlog() as they are unused now?

you are right. I removed them as well

[mlbridge]

Mailing list message from Andrew Haley on hotspot-compiler-dev:

On 1/18/24 09:29, Nick Gasson wrote:

Should we also remove `MacroAssembler::fast_log()` and `generate_dlog()` as they are unused now?

Yes, we shouldn't have code with no prospect of being used.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

[openjdk]

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

8210858: AArch64: remove Math.log intrinsic

Reviewed-by: ngasson, shade

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

• bde650f: 8322282: Incorrect LoaderConstraintTable::add_entry after JDK-8298468
• be943a9: 8321984: IGV: Upgrade to Netbeans Platform 20
• d3b2ac1: 8314186: runtime/8176717/TestInheritFD.java failed with "Log file was leaked"
• 72f1990: 8323057: Recoverable errors may be reported before unrecoverable errors when annotation processing is skipped
• c84af49: 8324129: C2: Remove some ttyLocker usages in preparation for JDK-8306767
• fd37262: 8323748: RISC-V: Add Zfh probe code
• 76afa02: 8322572: AllocationMergesTests.java fails with "IRViolationException: There were one or multiple IR rule failures."
• 2003610: 8323108: BufferedImage.setData(Raster) should not cast float and double values to integers
• a474b37: 8324240: Remove unused GrowableArrayView::EMPTY
• 68b5aab: 8324229: JFR: Temporarily disable assertion for missing object reference
• ... and 38 more: https://git.openjdk.org/jdk/compare/7be9f1d0540907f82800e717389bc3c2da3a8805...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.

[shipilev]

Looks good, with nits.

Separately, I do wonder if JDK-8301202 gives us a reason to avoid even calling to runtime, and instead just stay in Java completely.

[theRealAph]

Separately, I do wonder if JDK-8301202 gives us a reason to avoid even calling to runtime, and instead just stay in Java completely.

That would be nice.

[shipilev]

That would be nice.

https://bugs.openjdk.org/browse/JDK-8324296

[tobiasholenstein]

Separately, I do wonder if JDK-8301202 gives us a reason to avoid even calling to runtime, and instead just stay in Java completely.

Yes, would be nice to stay in Java. We had a similar discussion in #13606
where I benchmarked the intrinsics on different platforms. But, they still gave up to 40% performance improvement vs. staying in java. I don't think much has changed performance wise since then..

[tobiasholenstein]

Separately, I do wonder if JDK-8301202 gives us a reason to avoid even calling to runtime, and instead just stay in Java completely.

Yes, would be nice to stay in Java. We had a similar discussion in #13606 where I benchmarked the intrinsics on different platforms. But, they still gave up to 40% performance improvement vs. staying in java. I don't think much has changed performance wise since then..

@jddarcy any thoughts on that?

Perhaps I should re-run the benchmarks from May 2023..

[shipilev]

To be clear, the question on going to Java instead of runtime does not block this PR. I think the discussion on merits of removing the StubRoutines can continue in the relevant RFE.

[tobiasholenstein]

I think the discussion on merits of removing the StubRoutines can continue in the relevant RFE.

So move that discussion to https://bugs.openjdk.org/browse/JDK-8324296 ?

[shipilev]

I think the discussion on merits of removing the StubRoutines can continue in the relevant RFE.
So move that discussion to https://bugs.openjdk.org/browse/JDK-8324296 ?

Yes!

[theRealAph]

Separately, I do wonder if JDK-8301202 gives us a reason to avoid even calling to runtime, and instead just stay in Java completely.

Yes, would be nice to stay in Java. We had a similar discussion in #13606 where I benchmarked the intrinsics on different platforms. But, they still gave up to

"up to" isn't very interesting here. I think we care more about averages.

40% performance improvement vs. staying in java. I don't think much has changed performance wise since then..

@jddarcy any thoughts on that?

Perhaps I should re-run the benchmarks from May 2023..

That would be good. Significant performance deficits on purely numeric code demand explanation.

[tobiasholenstein]

Thanks for the reviews @shipilev and @nick-arm !

/integrate

[openjdk]

Going to push as commit 422020c.
Since your change was applied there have been 112 commits pushed to the master branch:

• 0d5f5e1: 8322484: 22-b26 Regression in J2dBench-bimg_misc-G1 (and more) on Windows-x64 and macOS-x64
• af9cd97: 8324800: gcc windows build broken after 8322757
• 8950d68: 8324753: [AIX] adjust os_posix after JDK-8318696
• 65d6bc1: 8324765: C2 SuperWord: remove dead code: SuperWord::insert_extracts
• 525c0cd: 8324752: C2 Superword: remove SuperWordRTDepCheck
• 2e748c9: 8175386: Clarify exception behavior of Types utility methods
• 70f4a4e: 8324786: validate-source fails after JDK-8042981
• ed3272c: 8042981: Strip type annotations in Types' utility methods
• 6d18562: 8324785: ProblemList two tests on linux due to JDK-8315923
• 91d8ea7: 8323835: Updating ASM to 9.6 for JDK 23
• ... and 102 more: https://git.openjdk.org/jdk/compare/7be9f1d0540907f82800e717389bc3c2da3a8805...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tobiasholenstein Pushed as commit 422020c.

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