8319700: [AArch64] C2 compilation fails with "Field too big for insn"

[xmas92]

Not all ZGC C2 BarrierStubs used on aarch64 participates in the laying out of trampoline stubs. (Used enable as many tbX instructions as possible.) This leads to to incorrect calculations which may cause the target offset for the tbX branch to become to large.

This fix changes all the BarriesStubs to stubs which participates in the trampoline logic.

Until more platforms requires specialised barrier stub layouts it is not worth adding better support for this pattern. Without a redesign it does make it harder to ensure that this is used correctly. For now the shared code asserts when building for aarch64 that the general shared stubs are not used directly. But care would still have to be taken if any new barrier stubs are introduced.

The behaviour was more easily reproducible when large inlining heuristics. This flag combination was used to get somewhat reliable reproducibility -esa -ea -XX:MaxInlineLevel=300 -XX:MaxInlineSize=1100 -XX:MaxTrivialSize=1000 -XX:LiveNodeCountInliningCutoff=1000000 -XX:MaxNodeLimit=3000000 -XX:NodeLimitFudgeFactor=600000 -XX:+UnlockExperimentalVMOptions -XX:+UseVectorStubs

There was also an observation inside the JBS comments that there where no tbX instructions branching to the emitted trampolines. However I was unable to reproduce this. Ran all tests with the following guarantee, this could not observe it either.

diff --git a/src/hotspot/cpu/aarch64/gc/z/zBarrierSetAssembler_aarch64.cpp b/src/hotspot/cpu/aarch64/gc/z/zBarrierSetAssembler_aarch64.cpp
index ebaf1829972..b6c40163a6b 100644
--- a/src/hotspot/cpu/aarch64/gc/z/zBarrierSetAssembler_aarch64.cpp
+++ b/src/hotspot/cpu/aarch64/gc/z/zBarrierSetAssembler_aarch64.cpp
@@ -36,6 +36,7 @@
 #include "runtime/icache.hpp"
 #include "runtime/jniHandles.hpp"
 #include "runtime/sharedRuntime.hpp"
+#include "utilities/debug.hpp"
 #include "utilities/macros.hpp"
 #ifdef COMPILER1
 #include "c1/c1_LIRAssembler.hpp"
@@ -1358,6 +1359,7 @@ void ZLoadBarrierStubC2Aarch64::emit_code(MacroAssembler& masm) {
   // Current assumption is that the barrier stubs are the first stubs emitted after the actual code
   assert(stubs_start_offset() <= output->buffer_sizing_data()->_code, "stubs are assumed to be emitted directly after code and code_size is a hard limit on where it can start");
 
+  guarantee(!_test_and_branch_reachable_entry.is_unused(), "Should be used");
   __ bind(_test_and_branch_reachable_entry);
 
   // Next branch's offset is unknown, but is > branch_offset

• Testing
  • linux-aarch64, linux-aarch64-debug,macosx-aarch64, macosx-aarch64-debug
    • ZGC tier1-tier7 Test Groups
    • ZGC tier1-tier7 Test Groups with large C2 inlining heuristics
      • (With test failures due to incompatible flags filtered out)

---

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-8319700: [AArch64] C2 compilation fails with "Field too big for insn" (Bug - P2)

Reviewers

• Andrew Haley (@theRealAph - Reviewer)
• Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16780

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@xmas92 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.

[mlbridge]

Webrevs

• 00: Full (dcfda1e6)

[theRealAph]

That looks like a reasonable thing to do. In C2 elsewhere we use assembler relaxation to allow the use of TBx even in large methods, but this is good enough for now.

[openjdk]

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

8319700: [AArch64] C2 compilation fails with "Field too big for insn"

Reviewed-by: aph, thartmann

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

• 864b39a: 8320564: RISC-V: Minimal build failed after JDK-8316592
• 2bb4b93: 8319569: Several java/util tests should be updated to accept VM flags
• 6016536: 8314745: JFR: @StackFilter
• aac4318: 8320577: Improve MessageHeader's toString() function to make HttpURLConnection's debug log readable
• 572b14a: 8320536: problemlist failing serviceability/attach/ConcAttachTest.java test on macosx
• 30462f9: 8318986: Improve GenericWaitBarrier performance
• 407cdd4: 8320207: doclet incorrectly chooses code font for a See Also link
• 1629a90: 8320331: G1 Full GC Heap verification relies on metadata not reset before verification
• 93bdc2a: 8306055: Add a built-in Catalog to JDK XML module
• a4bd9e4: 8319440: RISC-V: jdk can't be built with clang due to register keyword
• ... and 53 more: https://git.openjdk.org/jdk/compare/de51aa19d6a8cbd3b83bf469cb89da16f4b6f498...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.

[TobiHartmann]

Looks good to me too.

[xmas92]

It would be nice if the branch shortening for these stubs could be done as some fixpoint iteration. Or take part when laying out the blocks, as maybe even more branches could be shortened if all the BarrierStubs were not at the very end.

Thanks for the reviews.

/integrate

[openjdk]

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

• 99b9cb0: 8320586: update manual test/jdk/TEST.groups
• 8db7bad: 8319813: Remove upper limit on number of compiler phases in phasetype.hpp
• c49fb4f: 8320403: C2: PrintIdeal is no longer dumped to tty when xtty is set
• 06d957f: 8320582: Zero: Misplaced CX8 enablement flag
• 14193a0: 8314614: jdk/jshell/ImportTest.java failed with "InternalError: Failed remote listen"
• 864b39a: 8320564: RISC-V: Minimal build failed after JDK-8316592
• 2bb4b93: 8319569: Several java/util tests should be updated to accept VM flags
• 6016536: 8314745: JFR: @StackFilter
• aac4318: 8320577: Improve MessageHeader's toString() function to make HttpURLConnection's debug log readable
• 572b14a: 8320536: problemlist failing serviceability/attach/ConcAttachTest.java test on macosx
• ... and 58 more: https://git.openjdk.org/jdk/compare/de51aa19d6a8cbd3b83bf469cb89da16f4b6f498...master

Your commit was automatically rebased without conflicts.

[openjdk]

@xmas92 Pushed as commit 3787ff8.

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

[xmas92]

/backport jdk21u

[openjdk]

@xmas92 the backport was successfully created on the branch xmas92-backport-3787ff8d in my personal fork of openjdk/jdk21u. To create a pull request with this backport targeting openjdk/jdk21u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 3787ff8d from the openjdk/jdk repository.

The commit being backported was authored by Axel Boldt-Christmas on 23 Nov 2023 and was reviewed by Andrew Haley and Tobias Hartmann.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u:

$ git fetch https://github.com/openjdk-bots/jdk21u.git xmas92-backport-3787ff8d:xmas92-backport-3787ff8d
$ git checkout xmas92-backport-3787ff8d
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u.git xmas92-backport-3787ff8d

[theRealAph]

It would be nice if the branch shortening for these stubs could be done as some fixpoint iteration. Or take part when laying out the blocks, as maybe even more branches could be shortened if all the BarrierStubs were not at the very end.

Probably not worth making the effort for AArch64, because even the short branches are fairly long, so it takes quite a lot of work to find cases where they're exceeded. The few cases where it might help are rare indeed.