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

AArch64 macOS: Stop assigning x18 #18351

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

knn-k
Copy link
Contributor

@knn-k knn-k commented Oct 26, 2023

This commit stops the JIT assigning the x18 register.

This commit stops the JIT assigning the x18 register.

Signed-off-by: KONNO Kazuhiro <konno@jp.ibm.com>
@knn-k
Copy link
Contributor Author

knn-k commented Oct 26, 2023

Test jobs on AArch64 macOS with this change was successful.

  • JDK 11 sanity.functional: job/Test_openjdk11_j9_sanity.functional_aarch64_mac_Personal/124/
  • JDK 17 sanity.functional: job/Test_openjdk17_j9_sanity.functional_aarch64_mac_Personal/96/

Also waiting for a test jobs on AArch64 Linux to complete:

  • JDK 11 sanity.functional: job/Test_openjdk11_j9_sanity.functional_aarch64_linux_Personal/325/

@knn-k knn-k marked this pull request as ready for review October 26, 2023 12:14
@knn-k knn-k requested a review from 0xdaryl October 26, 2023 12:14
@pshipton
Copy link
Member

Does this change need to get into the 0.41 release? I"m not sure of the impact, but seems the JVM doesn't work on the latest macOS 13 versions?

@0xdaryl
Copy link
Contributor

0xdaryl commented Oct 27, 2023

Does this change need to get into the 0.41 release? I"m not sure of the impact, but seems the JVM doesn't work on the latest macOS 13 versions?

It could if there is still time. I hadn't considered it because I thought we were well past accepting new changes (even critical ones for that matter).

This change is not without risk though. It does change the generated code as different register assignment and spilling choices may be taken. Normally, I'd like a change of this nature to soak in a few test cycles rather than rushing into an already delayed release with no opportunity for remediation.

@0xdaryl
Copy link
Contributor

0xdaryl commented Oct 27, 2023

Because of the infra problems affecting macOS on Apple Silicon, @knn-k has been testing internally. I believe all looks good there, but I'd like him to confirm here before merging.

@0xdaryl 0xdaryl self-assigned this Oct 27, 2023
@knn-k
Copy link
Contributor Author

knn-k commented Oct 27, 2023

If this PR fixes Issue #18336, I think it is worth making it a go for v0.41.

Our internal machine macaarch64rt8 runs macOS 13, and the jobs on that machine are passing without this PR.

Examples:

  • Test_openjdk11_j9_sanity.functional_aarch64_mac/355/
  • Test_openjdk11_j9_extended.functional_aarch64_mac/355/
  • Test_openjdk11_j9_sanity.system_aarch64_mac/354/
  • Test_openjdk17_j9_sanity.functional_aarch64_mac/372/
  • Test_openjdk17_j9_extended.functional_aarch64_mac/370/
  • Test_openjdk17_j9_sanity.system_aarch64_mac/366/

On the other hand, the test job on AArch64 Linux with this PR above finished successfully.

@0xdaryl
Copy link
Contributor

0xdaryl commented Oct 27, 2023

This change is not without risk though. It does change the generated code as different register assignment and spilling choices may be taken. Normally, I'd like a change of this nature to soak in a few test cycles rather than rushing into an already delayed release with no opportunity for remediation.

As this is a user-raised issue, and -Xint isn't really a practical workaround, then I'd be willing to forgo these usual concerns and support it going into 0.41 if the release schedule allows for it.

@Akira1Saitoh
Copy link
Contributor

I am able to launch MemoryAnalyzer with the steps described in #18336 using the binary that includes this fix.

  • /job/Build_JDK17_aarch64_mac_Personal/187/

@pshipton
Copy link
Member

Ideally this is merged before the weekend testing starts Friday evening, and then we'll have those results.

I believe the internal machine is running 13.0, and the OpenJ9 machine is 13.4. I'm wondering if it's only a problem on newer versions.
@Akira1Saitoh what version is your laptop?

@Akira1Saitoh
Copy link
Contributor

It's Ventura 13.6.

knn-k added a commit to knn-k/openj9 that referenced this pull request Oct 27, 2023
This commit stops the JIT assigning the x18 register on AArch64 macOS.

Original PR in master: eclipse-openj9#18351

Signed-off-by: KONNO Kazuhiro <konno@jp.ibm.com>
@knn-k
Copy link
Contributor Author

knn-k commented Oct 27, 2023

I opened PR #18357 for v0.41.0-release.

@0xdaryl
Copy link
Contributor

0xdaryl commented Oct 27, 2023

This is a macOS on Arm specific change. This has been tested extensively internally. Given the infrastructure problems with macOS on Arm hardware in the open project a Jenkins CI test is pointless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants