8282322: AArch64: Provide a means to eliminate all STREX family of instructions #8779

dchuyko · 2022-05-18T19:05:03Z

On AArch64 it is sometimes convenient to have LSE atomics right from the start. Currently they are enabled after feature detection and RR reverse debugger works incorrectly.

New build configuration feature 'hardlse' is added. If it is enabled for aarch64 type of build, then statically compiled stubs replace the initial pessimistic implementation and dynamically generated replacements (when LSE support is detected). The feature works for builds of all debug levels.

New file atomic_linux_aarch64_lse.S is derived from atomic_linux_aarch64.S and inherits its copyright. This alternative static implementation corresponds to the dynamically generated code.

Note, this configuration part is necessary but not sufficient to fully avoid strex instructions for practical purposes. Other parts are:

Run on the OS built without strex family instructions. E.g. Amazon Linux 2022.
Compile with outline atomics enabled and the configuration flag enabled. E.g. configure with
--with-extra-cflags='-march=armv8.3-a+crc+crypto -moutline-atomics' --with-extra-cxxflags='-march=armv8.3-a+crc+crypto -moutline-atomics' --with-extra-ldflags='-Wl,--allow-multiple-definition' --with-jvm-features=hardlse

Testing: tier1, tier2 on linux-aarch64 release builds with feature off and feature on.

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-8282322: AArch64: Provide a means to eliminate all STREX family of instructions

Reviewers

Nick Gasson (@nick-arm - Reviewer) ⚠️ Review applies to 89aa16ff
Andrew Haley (@theRealAph - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8779

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

Using diff file

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

bridgekeeper · 2022-05-18T19:06:01Z

👋 Welcome back dchuyko! 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-05-18T19:09:41Z

@dchuyko The following labels will be automatically applied to this pull request:

build
hotspot

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

mlbridge · 2022-05-18T19:13:19Z

Webrevs

src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64_lse.S

theRealAph · 2022-05-19T07:59:56Z

This looks reasonable enough, but I take it that this would create an OpenJDK build that would not run on AArch64 systems without LSE instructions. Might it not be a better idea to build with outline-atomics, and use them in OpenJDK? That would also solve your problem, and we wouldn't have AArch64 Linux OpenJDK binaries which don't work on all systems.

Note that atomic_bsd_aarch64.hpp already does this.

I'm also concerned about code rot with options like this one. atomic_linux_aarch64_lse.S would not be tested in any standard configuration.

theRealAph · 2022-05-19T08:43:08Z

This looks reasonable enough, but I take it that this would create an OpenJDK build that would not run on AArch64 systems without LSE instructions.

Forget that, we already do so with extra-cflags.

nick-arm · 2022-05-19T08:43:55Z

What's the advantage of defining the new hardlse VM feature over using the existing __ARM_FEATURE_ATOMICS preprocessor symbol? Both GCC and Clang will define that with an appropriate -march value, which you're passing to configure anyway. You could then conditionally emit the LSE instructions in atomic_linux_aarch64.S based on that.

theRealAph · 2022-05-19T08:49:04Z

There is another way to achieve what you want, which is less disruptive and I believe more useful.

I just did this:

cp src/hotspot/os_cpu/bsd_aarch64/atomic_bsd_aarch64.hpp src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp

and built OpenJDK with your configury, and it achieves exactly what you want: a HotSpot that uses LSE throughout. If you, therefore, add an option to build with host compiler's atomics, just like BSD does, you'll get what you need with almost no code changes.

theRealAph · 2022-05-19T08:51:10Z

What's the advantage of defining the new hardlse VM feature over using the existing __ARM_FEATURE_ATOMICS preprocessor symbol? Both GCC and Clang will define that with an appropriate -march value, which you're passing to configure anyway. You could then conditionally emit the LSE instructions in atomic_linux_aarch64.S based on that.

That's not a bad idea either. In fact, maybe I prefer it to my own suggestion.

dchuyko · 2022-05-19T10:05:41Z

Thanks a lot for the great comments. Both suggestions make sense to me. The resulting code can be driven by the compiler flags in the configuration.

As I see it:

To have a single compile time implementation based on __atomic_add_fetch intrinsics (no .S file at all).
In generated part wrap put 'if (UseLSE)' blocks under '#ifdef __ARM_FEATURE_ATOMICS'.

Some concerns:

It should work at least for Clang and GCC but still will require checking all os-cpu/compiler variants.
Windows-aarch64 is the most worrisome.
The way of configuration is different for different compilers, not a problem though if we need an optimized build or to make rr work.

theRealAph · 2022-05-19T14:32:46Z

I literally just tried this:


        .text

#ifdef __ARM_FEATURE_ATOMICS

        .globl aarch64_atomic_fetch_add_8_default_impl
        .align 5
aarch64_atomic_fetch_add_8_default_impl:
        prfm    pstl1strm, [x0]
0:      ldaddal x1, x2, [x0]
        dmb     ish
        mov     x0, x2

with the obvious #else and #endif around the non-LSE part

and

$ objdump -d /home/aph/theRealAph-jdk/build/linux-aarch64-server-slowdebug/hotspot/variant-server/libjvm/objs/atomic_linux_aarch64.o | head -40

/home/aph/theRealAph-jdk/build/linux-aarch64-server-slowdebug/hotspot/variant-server/libjvm/objs/atomic_linux_aarch64.o:     file format elf64-littleaarch64


Disassembly of section .text:

0000000000000000 <aarch64_atomic_fetch_add_8_default_impl>:
   0:	f9800011 	prfm	pstl1strm, [x0]
   4:	f8e10002 	ldaddal	x1, x2, [x0]
   8:	d5033bbf 	dmb	ish
   c:	aa0203e0 	mov	x0, x2
  10:	d65f03c0 	ret

src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64_lse.S

theRealAph · 2022-05-19T14:55:21Z

Some concerns:

1. It should work at least for Clang and GCC but still will require checking all os-cpu/compiler variants.

Non-Linux systems don't use this stuff at all, so don't worry about it.

magicus

Configure files need a bit more work.

make/autoconf/jvm-features.m4

This reverts commit c5da85d.

dchuyko · 2022-05-23T16:08:17Z

New variant turns on using LSE atomics statically when __ARM_FEATURE_ATOMICS compiler feature is on. Prefetch is left only to non-LSE case, the kernel seems to do the same.

src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S

magicus · 2022-05-23T17:16:21Z

/label -build

openjdk · 2022-05-23T17:17:19Z

@magicus
The build label was successfully removed.

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp

src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S

nick-arm

Looks OK to me.

openjdk · 2022-06-09T08:11:44Z

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

8282322: AArch64: Provide a means to eliminate all STREX family of instructions

Reviewed-by: ngasson, aph

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

1fec62f: 8289710: Move Suspend/Resume classes out of os.hpp
01b9f95: Merge
3212dc9: 8289486: Improve XSLT XPath operators count efficiency
a8eb728: 8289779: Map::replaceAll javadoc has redundant @throws clauses
889150b: 8289558: Need spec clarification of j.l.foreign.*Layout
8dd94a2: 8289196: Pattern domination not working properly for record patterns
8f24d25: 6509045: {@inheritdoc} only copies one instance of the specified exception
9a0fa82: 8288949: serviceability/jvmti/vthread/ContStackDepthTest/ContStackDepthTest.java failing
55fa19b: 8289857: ProblemList jdk/jfr/event/runtime/TestActiveSettingEvent.java
32b650c: 8289840: ProblemList vmTestbase/nsk/jdwp/ThreadReference/ForceEarlyReturn/forceEarlyReturn002/forceEarlyReturn002.java when run with vthread wrapper
... and 251 more: https://git.openjdk.org/jdk/compare/47b86690b6672301aa46d4a7b9ced58d17047cc7...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.

src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S

theRealAph · 2022-07-07T16:09:57Z

Is this dead, or... ?

phohensee · 2022-07-07T18:20:32Z

Andrew, looks like Dima will be good to go once you re-review it.

dchuyko · 2022-07-07T21:31:27Z

@theRealAph I have made the latest edits as per your comments and they are pending review.

theRealAph · 2022-07-08T07:10:21Z

Andrew, looks like Dima will be good to go once you re-review it.

Fine by me, but note where we are in rampdown.

dchuyko · 2022-07-08T08:53:47Z

/integrate

openjdk · 2022-07-08T08:55:23Z

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

f1967cf: 8289997: gc/g1/TestVerificationInConcurrentCycle.java fails due to use of debug-only option
1b8f466: 8289740: Add verification testing during all concurrent phases in G1
ac399e9: 8286957: Held monitor count
1fec62f: 8289710: Move Suspend/Resume classes out of os.hpp
01b9f95: Merge
3212dc9: 8289486: Improve XSLT XPath operators count efficiency
a8eb728: 8289779: Map::replaceAll javadoc has redundant @throws clauses
889150b: 8289558: Need spec clarification of j.l.foreign.*Layout
8dd94a2: 8289196: Pattern domination not working properly for record patterns
8f24d25: 6509045: {@inheritdoc} only copies one instance of the specified exception
... and 254 more: https://git.openjdk.org/jdk/compare/47b86690b6672301aa46d4a7b9ced58d17047cc7...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-08T08:55:37Z

@dchuyko Pushed as commit a13af65.

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

dchuyko · 2022-07-08T08:57:25Z

Andrew, thanks for taking a look. This change is now for the master, later we can also consider update releases.

theRealAph · 2022-07-09T09:27:53Z

Andrew, thanks for taking a look. This change is now for the master, later we can also consider update releases.

You'll have to take your chances with that. I'm not sure that it qualifies for a backport under any of the usual criteria, but we can discuss that.

dchuyko added 2 commits May 18, 2022 21:39

hardlse feature

c5da85d

Merge branch 'openjdk:master' into JDK-8282322

87b3cbb

openjdk bot changed the title ~~JDK-8282322~~ 8282322: AArch64: Provide a means to eliminate all STREX family of instructions May 18, 2022

openjdk bot added the rfr Pull request is ready for review label May 18, 2022

openjdk bot added build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels May 18, 2022

theRealAph reviewed May 19, 2022

View reviewed changes

src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64_lse.S Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

theRealAph reviewed May 19, 2022

View reviewed changes

src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64_lse.S Outdated Show resolved Hide resolved

magicus suggested changes May 20, 2022

View reviewed changes

make/autoconf/jvm-features.m4 Outdated Show resolved Hide resolved

make/autoconf/jvm-features.m4 Outdated Show resolved Hide resolved

dchuyko added 2 commits May 23, 2022 18:59

Revert "hardlse feature"

d47d14c

This reverts commit c5da85d.

Use LSE in linux-aarch64 asm code if __ARM_FEATURE_ATOMICS is on

1287cce

Merge branch 'openjdk:master' into JDK-8282322

f7c8378

theRealAph reviewed May 23, 2022

View reviewed changes

src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S Show resolved Hide resolved

openjdk bot removed the build build-dev@openjdk.org label May 23, 2022

theRealAph reviewed May 24, 2022

View reviewed changes

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp Outdated Show resolved Hide resolved

theRealAph reviewed May 24, 2022

View reviewed changes

src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S Outdated Show resolved Hide resolved

dchuyko added 2 commits June 8, 2022 12:30

Merge branch 'openjdk:master' into JDK-8282322

adef7ab

Removed unnecessary changes (forced UseLSE, blank lines)

89aa16f

nick-arm approved these changes Jun 9, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 9, 2022

theRealAph reviewed Jun 9, 2022

View reviewed changes

src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S Outdated Show resolved Hide resolved

theRealAph reviewed Jun 9, 2022

View reviewed changes

src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S Outdated Show resolved Hide resolved

dchuyko added 4 commits June 16, 2022 15:24

Moved 2 prfm-s from wrong ifdef branch

653ebf4

Merge branch 'openjdk:master' into JDK-8282322

018172c

Merge branch 'openjdk:master' into JDK-8282322

9e5a1ee

Merge branch 'openjdk:master' into JDK-8282322

d1ae97d

theRealAph approved these changes Jul 8, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 8, 2022

openjdk bot closed this Jul 8, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8282322: AArch64: Provide a means to eliminate all STREX family of instructions #8779

8282322: AArch64: Provide a means to eliminate all STREX family of instructions #8779

dchuyko commented May 18, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 18, 2022

openjdk bot commented May 18, 2022

mlbridge bot commented May 18, 2022 •

edited

Loading

theRealAph commented May 19, 2022

theRealAph commented May 19, 2022

nick-arm commented May 19, 2022

theRealAph commented May 19, 2022

theRealAph commented May 19, 2022

dchuyko commented May 19, 2022

theRealAph commented May 19, 2022 •

edited

Loading

This comment was marked as outdated.

theRealAph commented May 19, 2022

magicus left a comment

dchuyko commented May 23, 2022

magicus commented May 23, 2022

openjdk bot commented May 23, 2022

nick-arm left a comment

openjdk bot commented Jun 9, 2022 •

edited

Loading

theRealAph commented Jul 7, 2022

phohensee commented Jul 7, 2022

dchuyko commented Jul 7, 2022

theRealAph commented Jul 8, 2022

dchuyko commented Jul 8, 2022

openjdk bot commented Jul 8, 2022

openjdk bot commented Jul 8, 2022

dchuyko commented Jul 8, 2022

theRealAph commented Jul 9, 2022

8282322: AArch64: Provide a means to eliminate all STREX family of instructions #8779

8282322: AArch64: Provide a means to eliminate all STREX family of instructions #8779

Conversation

dchuyko commented May 18, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 18, 2022

openjdk bot commented May 18, 2022

mlbridge bot commented May 18, 2022 • edited Loading

Webrevs

theRealAph commented May 19, 2022

theRealAph commented May 19, 2022

nick-arm commented May 19, 2022

theRealAph commented May 19, 2022

theRealAph commented May 19, 2022

dchuyko commented May 19, 2022

theRealAph commented May 19, 2022 • edited Loading

This comment was marked as outdated.

theRealAph commented May 19, 2022

magicus left a comment

Choose a reason for hiding this comment

dchuyko commented May 23, 2022

magicus commented May 23, 2022

openjdk bot commented May 23, 2022

nick-arm left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 9, 2022 • edited Loading

theRealAph commented Jul 7, 2022

phohensee commented Jul 7, 2022

dchuyko commented Jul 7, 2022

theRealAph commented Jul 8, 2022

dchuyko commented Jul 8, 2022

openjdk bot commented Jul 8, 2022

openjdk bot commented Jul 8, 2022

dchuyko commented Jul 8, 2022

theRealAph commented Jul 9, 2022

dchuyko commented May 18, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented May 18, 2022 •

edited

Loading

theRealAph commented May 19, 2022 •

edited

Loading

openjdk bot commented Jun 9, 2022 •

edited

Loading