Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8271128: InlineIntrinsics support for 32-bit ARM #4927

Closed

Conversation

mychris
Copy link
Member

@mychris mychris commented Jul 29, 2021

Hi,

please review this patch, which adds support for InlineIntrinsics to the 32-bit ARM port. The old aarch32 port had this intrinsic implemented and enabled by default.

Like on many other platforms, the 32-bit ARM port simply calls into the SharedRuntime to intrinsify the basic java.lang.Math methods. InlineIntrinsics is already implemented for C1 on 32-bit ARM, which does the same thing.

testing: hotspot tier1 on ARMv5TE (soft-float) and ARMv7-A (hard-float)

There is already the micro benchmark test/micro/org/openjdk/bench/java/lang/MathBench.java which I used. The soft-float benchmarks are not that meaningful, since I performed them in QEMU.

hard-float -Xint -XX:+InlineIntrinsics

Benchmark (seed) Mode Cnt Score Error Units
MathBench.absDouble 0 thrpt 5 1169.574 +/- 133.694 ops/ms
MathBench.cosDouble 0 thrpt 5 759.902 +/- 573.852 ops/ms
MathBench.expDouble 0 thrpt 5 854.753 +/- 67.217 ops/ms
MathBench.log10Double 0 thrpt 5 902.034 +/- 22.413 ops/ms
MathBench.logDouble 0 thrpt 5 895.470 +/- 113.811 ops/ms
MathBench.powDouble 0 thrpt 5 936.136 +/- 40.661 ops/ms
MathBench.sinDouble 0 thrpt 5 864.670 +/- 68.329 ops/ms
MathBench.sqrtDouble 0 thrpt 5 1082.589 +/- 92.570 ops/ms
MathBench.tanDouble 0 thrpt 5 853.715 +/- 122.427 ops/ms

hard-float -Xint -XX:-InlineIntrinsics

Benchmark (seed) Mode Cnt Score Error Units
MathBench.absDouble 0 thrpt 5 450.907 +/- 10.402 ops/ms
MathBench.cosDouble 0 thrpt 5 592.242 +/- 14.011 ops/ms
MathBench.expDouble 0 thrpt 5 167.614 +/- 7.530 ops/ms
MathBench.log10Double 0 thrpt 5 572.099 +/- 55.089 ops/ms
MathBench.logDouble 0 thrpt 5 596.588 +/- 24.976 ops/ms
MathBench.powDouble 0 thrpt 5 212.673 +/- 4.060 ops/ms
MathBench.sinDouble 0 thrpt 5 584.873 +/- 42.774 ops/ms
MathBench.sqrtDouble 0 thrpt 5 514.690 +/- 30.568 ops/ms
MathBench.tanDouble 0 thrpt 5 566.586 +/- 23.995 ops/ms

soft-float -Xint -XX:+InlineIntrinsics

Benchmark (seed) Mode Cnt Score Error Units
MathBench.absDouble 0 thrpt 5 279.575 +/- 56.455 ops/ms
MathBench.cosDouble 0 thrpt 5 137.005 +/- 72.561 ops/ms
MathBench.expDouble 0 thrpt 5 117.778 +/- 30.186 ops/ms
MathBench.log10Double 0 thrpt 5 107.957 +/- 10.158 ops/ms
MathBench.logDouble 0 thrpt 5 101.341 +/- 3.914 ops/ms
MathBench.powDouble 0 thrpt 5 222.220 +/- 3.854 ops/ms
MathBench.sinDouble 0 thrpt 5 112.715 +/- 9.088 ops/ms
MathBench.sqrtDouble 0 thrpt 5 119.341 +/- 76.528 ops/ms
MathBench.tanDouble 0 thrpt 5 105.224 +/- 30.477 ops/ms

soft-float -Xint -XX:-InlineIntrinsics

Benchmark (seed) Mode Cnt Score Error Units
MathBench.absDouble 0 thrpt 5 173.150 +/- 36.279 ops/ms
MathBench.cosDouble 0 thrpt 5 129.774 +/- 8.795 ops/ms
MathBench.expDouble 0 thrpt 5 53.524 +/- 1.679 ops/ms
MathBench.log10Double 0 thrpt 5 132.503 +/- 4.274 ops/ms
MathBench.logDouble 0 thrpt 5 135.483 +/- 1.150 ops/ms
MathBench.powDouble 0 thrpt 5 54.266 +/- 0.699 ops/ms
MathBench.sinDouble 0 thrpt 5 105.636 +/- 4.647 ops/ms
MathBench.sqrtDouble 0 thrpt 5 204.550 +/- 7.206 ops/ms
MathBench.tanDouble 0 thrpt 5 101.072 +/- 3.701 ops/ms

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4927/head:pull/4927
$ git checkout pull/4927

Update a local copy of the PR:
$ git checkout pull/4927
$ git pull https://git.openjdk.java.net/jdk pull/4927/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4927

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4927.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 29, 2021

👋 Welcome back cgo! 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 openjdk bot added the rfr label Jul 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 29, 2021

@mychris The following label will be automatically applied to this pull request:

  • hotspot

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.

@openjdk openjdk bot added the hotspot label Jul 29, 2021
@mychris
Copy link
Member Author

@mychris mychris commented Jul 29, 2021

/cc hotspot-runtime

@openjdk openjdk bot added the hotspot-runtime label Jul 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 29, 2021

@mychris
The hotspot-runtime label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 29, 2021

Webrevs

Copy link
Contributor

@shipilev shipilev left a comment

This looks okay to me, but I think hotspot-tier1 testing is inadequate for this. Please run at least full tier1, which should include JDK tests for java.lang.Math.

@mychris
Copy link
Member Author

@mychris mychris commented Jul 29, 2021

Sorry, I forgot to mention this.
I tested the java.lang.Math methods for which the new intrinsics are implemented manually, by comparing the results of the intrinsics with the results of the corresponding java.lang.StrictMath method. Since both, StrictMath and the intrinsics use the same algorithm on 32-bit ARM, it is possible to do that.
But I agree, doing some more testing doesn't hurt. I will start jdk tier1 as well and will report back as soon as they are done, will definitely take some time though.

@mychris
Copy link
Member Author

@mychris mychris commented Aug 4, 2021

jdk:tier1 tests on soft-float and hard-float are good. IeeeRecommendedTests on soft-float has some issues, but this is because I don't use an external soft float library, but the glibc implementation for floating point arithmetic, which doesn't have the required precision. Probably one should pick up JDK-8215902 again and fix this situation.

@shipilev
Copy link
Contributor

@shipilev shipilev commented Aug 4, 2021

jdk:tier1 tests on soft-float and hard-float are good.

Thanks, we are good then.

IeeeRecommendedTests on soft-float has some issues, but this is because I don't use an external soft float library, but the glibc implementation for floating point arithmetic, which doesn't have the required precision.

This is pre-existing, right? Meaning, it is not worse with this PR?

@mychris
Copy link
Member Author

@mychris mychris commented Aug 4, 2021

IeeeRecommendedTests on soft-float has some issues, but this is because I don't use an external soft float library, but the glibc implementation for floating point arithmetic, which doesn't have the required precision.

This is pre-existing, right? Meaning, it is not worse with this PR?

Yes, this PR doesn't change anything. The test case also uses the Math and StrictMath functions and the problem is identical for both implementations.

Copy link
Contributor

@shipilev shipilev left a comment

Good to go then.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 4, 2021

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

8271128: InlineIntrinsics support for 32-bit ARM

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

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 (@shipilev) 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).

@openjdk openjdk bot added the ready label Aug 4, 2021
@mychris
Copy link
Member Author

@mychris mychris commented Aug 5, 2021

Thanks for the review Aleksey, very much appreciated.
/integrate

@openjdk openjdk bot added the sponsor label Aug 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 5, 2021

@mychris
Your change (at version d8926df) is now ready to be sponsored by a Committer.

@shipilev
Copy link
Contributor

@shipilev shipilev commented Aug 5, 2021

Sorry for not catching this earlier, but I think java_lang_math_abs and java_lang_math_sqrt are not transcendental, so it is awkward to call generate_transcendental_entry for them? I think that's just a naming problem, maybe call it generate_math_runtime_entry? (If you fix this, maybe also pull from master to get clean GHA runs).

mychris added 2 commits Aug 5, 2021
* master: (97 commits)
  8270903: sun.net.httpserver.HttpConnection: Improve toString
  8271722: [TESTBUG] gc/g1/TestMixedGCLiveThreshold.java can fail if G1 Full GC uses >1 workers
  8270058: Use Objects.check{Index,FromIndexSize} for java.desktop
  4819544: SwingSet2 JTable Demo throws NullPointerException
  8271878: UnProblemList jdk/jfr/event/gc/detailed/TestEvacuationFailedEvent.java in JDK18
  8271895: UnProblemList javax/swing/JComponent/7154030/bug7154030.java in JDK18
  8271863: ProblemList serviceability/sa/TestJmapCore.java on linux-x64 with ZGC
  8271898: disable os.release_multi_mappings_vm on macOS-X64
  8271893: mark hotspot runtime/PerfMemDestroy/PerfMemDestroy.java test as ignoring external VM flags
  8271887: mark hotspot runtime/CDSCompressedKPtrs tests which ignore external VM flags
  8271891: mark hotspot runtime/Safepoint tests which ignore external VM flags
  8271886: mark hotspot runtime/InvocationTests tests which ignore external VM flags
  8271890: mark hotspot runtime/Dictionary tests which ignore external VM flags
  8271894: ProblemList javax/swing/JComponent/7154030/bug7154030.java in JDK17
  8271888: build error after JDK-8271599
  8271456: Avoid looking up standard charsets in "java.desktop" module
  8271589: fatal error with variable shift count integer rotate operation.
  8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in some places
  8271877: ProblemList jdk/jfr/event/gc/detailed/TestEvacuationFailedEvent.java in JDK17
  8271217: Fix race between G1PeriodicGCTask checks and GC request
  ...
@openjdk openjdk bot removed the sponsor label Aug 5, 2021
@mychris
Copy link
Member Author

@mychris mychris commented Aug 5, 2021

Sorry for not catching this earlier, but I think java_lang_math_abs and java_lang_math_sqrt are not transcendental, so it is awkward to call generate_transcendental_entry for them? I think that's just a naming problem, maybe call it generate_math_runtime_entry? (If you fix this, maybe also pull from master to get clean GHA runs).

I chose that name because of the aarch64 implementation, which uses the same name (but doesn't generate calls for neither abs, nor sqrt). I renamed it to generate_math_runtime_call, since it only generates the call to the runtime function.

@shipilev
Copy link
Contributor

@shipilev shipilev commented Aug 5, 2021

Sorry for not catching this earlier, but I think java_lang_math_abs and java_lang_math_sqrt are not transcendental, so it is awkward to call generate_transcendental_entry for them? I think that's just a naming problem, maybe call it generate_math_runtime_entry? (If you fix this, maybe also pull from master to get clean GHA runs).

I chose that name because of the aarch64 implementation, which uses the same name (but doesn't generate calls for neither abs, nor sqrt). I renamed it to generate_math_runtime_call, since it only generates the call to the runtime function.

Thanks, that looks better. transcendental_entry local variable too?

@mychris
Copy link
Member Author

@mychris mychris commented Aug 6, 2021

Thanks, that looks better. transcendental_entry local variable too?

Sorry, should have seen this myself.
Done.

Copy link
Contributor

@shipilev shipilev left a comment

I'd prefer use_runtime_function to be called use_runtime_call, but this is fine as well. I can sponsor this for you.

@mychris
Copy link
Member Author

@mychris mychris commented Aug 6, 2021

I'd prefer use_runtime_function to be called use_runtime_call

Done.

I can sponsor this for you.

Thanks, I guess I have to integrate again?

/integrate

@openjdk openjdk bot added the sponsor label Aug 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 6, 2021

@mychris
Your change (at version 6de922f) is now ready to be sponsored by a Committer.

@shipilev
Copy link
Contributor

@shipilev shipilev commented Aug 6, 2021

Yup, I'll wait for GHA to clear the ARM build, and then sponsor.

@shipilev
Copy link
Contributor

@shipilev shipilev commented Aug 6, 2021

ARM builds are clean.

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Aug 6, 2021

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

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Aug 6, 2021
@openjdk openjdk bot added integrated and removed ready rfr sponsor labels Aug 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 6, 2021

@shipilev @mychris Pushed as commit b6a19f1.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-runtime integrated
2 participants