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

8320682: [AArch64] C1 compilation fails with "Field too big for insn" #16951

Closed
wants to merge 6 commits into from

Conversation

dlunde
Copy link
Member

@dlunde dlunde commented Dec 4, 2023

This changeset fixes an issue where addresses for float and double constants on aarch64 were sometimes out of range for PC-relative offsets using adr.

Changes:

  • Set an upper bound of 1M for the flag NMethodSizeLimit, ensuring that float and double constants are in range for adr.
  • Revise tests in TestC1Globals.java to use the new upper bound of 1M for NMethodSizeLimit. Also, remove no longer applicable tests in TestC1Globals.java.

Testing (in progress)

Platforms: windows-x64, linux-x64, linux-aarch64, macosx-x64, macosx-aarch64

  • tier1, tier2, tier3, tier4, tier5
  • Targeted and repeated tests for TestC1Globals.java in all tiers

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

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16951

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 4, 2023

👋 Welcome back dlunde! 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 changed the title 8320682 8320682: [AArch64] C1 compilation fails with "Field too big for insn" Dec 4, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 4, 2023
@openjdk
Copy link

openjdk bot commented Dec 4, 2023

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

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 4, 2023
@mlbridge
Copy link

mlbridge bot commented Dec 4, 2023

Webrevs

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@openjdk
Copy link

openjdk bot commented Dec 4, 2023

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

8320682: [AArch64] C1 compilation fails with "Field too big for insn"

Reviewed-by: thartmann, aph, dlong

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

  • 5a97dbf: 8322034: Parallel: Remove unused methods in PSAdaptiveSizePolicy
  • 2838a91: 8288989: Make tests not depend on the source code
  • d2ba3b1: 8312150: Remove -Xnoagent option
  • d632d74: 8321820: TestLoadNIdeal fails on 32-bit because -XX:+UseCompressedOops is not recognized
  • ddbbd36: 8320279: Link issues in java.xml module-info.java
  • c8ad7b7: 8321974: Crash in ciKlass::is_subtype_of because TypeAryPtr::_klass is not initialized
  • cf94854: 8321565: [REDO] Heap dump does not contain virtual Thread stack references
  • 7ece9e9: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
  • 9320ef9: 8321973: Parallel: Remove unused methods in AdaptiveSizePolicy
  • 2a565ff: 8321808: G1: Use unsigned type for non-negative G1 flags
  • ... and 139 more: https://git.openjdk.org/jdk/compare/ed5b8c3a7bb6de27ab5050db494b08d5e5dd1c44...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, @theRealAph, @dean-long) 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 Pull request is ready to be integrated label Dec 4, 2023
@theRealAph
Copy link
Contributor

This is fine for C1. Iif it were for C2, we're already doing a relaxation pass which we could utilize to fix up out-of-range loads.

@dlunde
Copy link
Member Author

dlunde commented Dec 6, 2023

Thanks for the reviews @TobiHartmann and @theRealAph. Integrating now, but I need a sponsor.

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 6, 2023
@openjdk
Copy link

openjdk bot commented Dec 6, 2023

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

@theRealAph
Copy link
Contributor

It doesn't seem to have been integrated. Wait a little while please, there's something I'd like to try first.

@dlunde
Copy link
Member Author

dlunde commented Dec 6, 2023

It doesn't seem to have been integrated. Wait a little while please, there's something I'd like to try first.

Sure, I removed my integrate command and Roberto also removed his sponsor. Hopefully, that'll stop the integration.

@theRealAph
Copy link
Contributor

theRealAph commented Dec 6, 2023

This fix generates too much code. Please do this instead in both cases:

@@ -585,8 +585,8 @@ void LIR_Assembler::const2reg(LIR_Opr src, LIR_Opr dest, LIR_PatchCode patch_cod
       if (__ operand_valid_for_float_immediate(c->as_jdouble())) {
         __ fmovd(dest->as_double_reg(), (c->as_jdouble()));
       } else {
-        __ adr(rscratch1, InternalAddress(double_constant(c->as_jdouble())));
-        __ ldrd(dest->as_double_reg(), Address(rscratch1));
+        __ mov(rscratch1, jlong_cast(c->as_jdouble()));
+        __ fmovd(dest->as_double_reg(), rscratch1);
       }
       break;
     }

@theRealAph
Copy link
Contributor

I don't think this is the only place where we assume that a single method will be less than a megabyte when compiled. What triggered it?

@dlunde
Copy link
Member Author

dlunde commented Dec 6, 2023

Thanks Andrew. The trigger for this issue was a test case that I added as part of JDK-8318817 (in TestC1Globals.java). Specifically, the test uses a very large value for -XX:NMethodSizeLimit, which causes the non-nmethod code heap to take up most of the code cache (leaving the profiled and non-profiled code heaps at minimum size, usually 4KB).

If there is an implicit assumption regarding the cache size in more places, we should perhaps not integrate this fix and instead ensure the cache size is always properly sized. We are, for example, considering setting some more reasonable upper bound for -XX:NMethodSizeLimit. What do you think Andrew?

@theRealAph
Copy link
Contributor

The PC-relative ldr() instruction is designed for exactly this use case, and its range is +/-1MB. We use this form for constant loads into SIMD and FP registers, in C1 and C2. We could also use it for other constant loads too, and the code is there to do so, but we don't at the present time.

@theRealAph
Copy link
Contributor

The PC-relative ldr() instruction...

Sorry, PC-relative adr(), but the same +/-1MB, and the reason for it, still applies.

@dlunde
Copy link
Member Author

dlunde commented Dec 6, 2023

Given the +/-1MB range for adr, would it instead be a good idea to limit -XX:NMethodSizeLimit to 1MB? Then we would not need the fix at all and could still use adr.

Something similar to:

diff --git a/src/hotspot/share/c1/c1_globals.hpp b/src/hotspot/share/c1/c1_globals.hpp
index 1c22cf16cfe..e2057d20e59 100644
--- a/src/hotspot/share/c1/c1_globals.hpp
+++ b/src/hotspot/share/c1/c1_globals.hpp
@@ -277,7 +277,7 @@
                                                                             \
   develop(intx, NMethodSizeLimit, (64*K)*wordSize,                          \
           "Maximum size of a compiled method.")                             \
-          range(0, max_jint)                                                \
+          range(0, 1*M)                                                     \
                                                                             \
   develop(bool, TraceFPUStack, false,                                       \
           "Trace emulation of the FPU stack (intel only)")                  \

@theRealAph
Copy link
Contributor

Given the +/-1MB range for adr, would it instead be a good idea to limit -XX:NMethodSizeLimit to 1MB?

I guess that would be OK. I'm thinking of extreme scenarios where a method's constant pool is large but the stub code at its end is smaller, in which case an adr wouldn't quite reach. But I think that's unlikely.

@openjdk openjdk bot removed the sponsor Pull request is ready to be sponsored label Dec 7, 2023
@dlunde
Copy link
Member Author

dlunde commented Dec 7, 2023

@theRealAph @TobiHartmann: I've now reverted the fix and instead set an upper bound for NMethodSizeLimit. Preliminary testing shows no issues, but I'm also running more tests before integrating.

Requesting a re-review, thanks.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 1M limit seems reasonable to me given that NMethodSizeLimit is a develop flag and the default of 64K is much lower.

@dean-long
Copy link
Member

The 1M limit seems reasonable to me given that NMethodSizeLimit is a develop flag and the default of 64K is much lower.

Good point about it being a developer flag. I missed that. But the default is 64K words, or half of 1M.

@TobiHartmann
Copy link
Member

But the default is 64K words, or half of 1M.

Right, I missed that.

@dlunde
Copy link
Member Author

dlunde commented Dec 14, 2023

Thanks for the input everyone. Let's go with a 1MB upper bound for now then (on all platforms). I'll rerun some tests before integrating.

@dlunde
Copy link
Member Author

dlunde commented Dec 14, 2023

Ready now, please sponsor.

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 14, 2023
@openjdk
Copy link

openjdk bot commented Dec 14, 2023

@dlunde
Your change (at version 1d0515b) is now ready to be sponsored by a Committer.

@robcasloz
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented Dec 14, 2023

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

  • 5a97dbf: 8322034: Parallel: Remove unused methods in PSAdaptiveSizePolicy
  • 2838a91: 8288989: Make tests not depend on the source code
  • d2ba3b1: 8312150: Remove -Xnoagent option
  • d632d74: 8321820: TestLoadNIdeal fails on 32-bit because -XX:+UseCompressedOops is not recognized
  • ddbbd36: 8320279: Link issues in java.xml module-info.java
  • c8ad7b7: 8321974: Crash in ciKlass::is_subtype_of because TypeAryPtr::_klass is not initialized
  • cf94854: 8321565: [REDO] Heap dump does not contain virtual Thread stack references
  • 7ece9e9: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
  • 9320ef9: 8321973: Parallel: Remove unused methods in AdaptiveSizePolicy
  • 2a565ff: 8321808: G1: Use unsigned type for non-negative G1 flags
  • ... and 139 more: https://git.openjdk.org/jdk/compare/ed5b8c3a7bb6de27ab5050db494b08d5e5dd1c44...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 14, 2023
@openjdk openjdk bot closed this Dec 14, 2023
@openjdk 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 Dec 14, 2023
@openjdk
Copy link

openjdk bot commented Dec 14, 2023

@robcasloz @dlunde Pushed as commit 69014cd.

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

@dlunde
Copy link
Member Author

dlunde commented Dec 14, 2023

/backport jdk22

@openjdk
Copy link

openjdk bot commented Dec 14, 2023

@dlunde To use the /backport command, you need to be in the OpenJDK census and your GitHub account needs to be linked with your OpenJDK username (how to associate your GitHub account with your OpenJDK username).

@TobiHartmann
Copy link
Member

/backport jdk22

@openjdk
Copy link

openjdk bot commented Dec 15, 2023

@TobiHartmann the backport was successfully created on the branch backport-TobiHartmann-69014cd5 in my personal fork of openjdk/jdk22. To create a pull request with this backport targeting openjdk/jdk22: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 69014cd5 from the openjdk/jdk repository.

The commit being backported was authored by Daniel Lundén on 14 Dec 2023 and was reviewed by Tobias Hartmann, Andrew Haley and Dean Long.

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/jdk22:

$ git fetch https://github.com/openjdk-bots/jdk22.git backport-TobiHartmann-69014cd5:backport-TobiHartmann-69014cd5
$ git checkout backport-TobiHartmann-69014cd5
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk22.git backport-TobiHartmann-69014cd5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
5 participants