Skip to content

Disable TLH prefetching for portable AOT code #20027

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

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

mpirvu
Copy link
Contributor

@mpirvu mpirvu commented Aug 20, 2024

Current code for x86 disables TLH prefething globally for Intel CPUs newer than Broadwell architecture and enables it globally for Broadwell or older CPUs.
If a container image is generated on a newer architecture, then TLH prefetch will be off and this information is written into the AOT header for the shared class cache embedded in the container. If that container image is run on an older architecture, the JVM will set TLH prefetch off, and the AOT compatibility check will fail, rendering the embedded AOT code useless.

This commit disables TLH prefething for portable AOT code.

Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

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

Overall LGTM, minor change requested.

@dsouzai dsouzai self-assigned this Aug 21, 2024
@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 21, 2024

On a cascadelake machine I created two OpenJ9 containers, one with no fix and one with the fix from this PR.
I moved the images on a fyre VM which indicates that the processor is Broadwell. The container without the fix shows the AOT incompatibility die TLH-prefetch mismatch. The container with fix does not this issue and can load AOT code from the embedded SCC.

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 21, 2024

jenkins test sanity all jdk17

@dsouzai
Copy link
Contributor

dsouzai commented Aug 21, 2024

[2024-08-21T16:42:31.536Z] /Users/jenkins/workspace/Build_JDK17_aarch64_mac_Personal/openj9/runtime/compiler/aarch64/codegen/J9TreeEvaluator.cpp:2937:12: error: no member named 'enableTLHPrefetching' in 'TR::CodeGenerator'
[2024-08-21T16:42:31.536Z]    if (cg->enableTLHPrefetching() && (!isTooSmallToPrefetch))
[2024-08-21T16:42:31.536Z]        ~~  ^
[2024-08-21T16:42:31.536Z] 1 error generated.

I guess that flag is defined on a per-codegen basis..., in OMR no less..

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 21, 2024

Looking at the code, there are many inconsistencies:

  • Codegens on X/P/Z test cg->enableTLHPrefetching() which is set during codegen initialization. aarch64 codegen tests the TR_TLHPrefetch option directly
  • Codegens on P/Z do not set enableTLHPrefetching for AOT compilations. X and aarch64 don't seem to care about AOT
  • When setting enableTLHPrefetching, codegens on X and P test the TR_TLHPrefetch option. However, the codegen on Z tests a different option: TR_DisableTLHPrefetch

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 21, 2024

I will fix the build error on aarch64 in this PR by using the old code. We may want to create a 0.46.1 release with this fix and changing the OMR component will complicate things.

In separate PRs (omr and openj9) I will address the inconsistencies above.

Current code for x86 disables TLH prefething globally for
Intel CPUs newer than Broadwell architecture and enables it
globally for Broadwell or older CPUs.
If a container image is generated on a newer architecture,
then TLH prefetch will be off and this information is written
into the AOT header for the shared class cache embedded in the
container. If that container image is run on an older architecture,
the JVM will set TLH prefetch off, and the AOT compatibility
check will fail, rendering the embedded AOT code useless.

This commit disables TLH prefething for portable AOT code.

Signed-off-by: Marius Pirvu <[email protected]>
@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 22, 2024

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 22, 2024

jenkins test sanity amac,alinux,win64 jdk17

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 22, 2024

jenkins test sanity amac,alinux,win jdk17

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 22, 2024

alinux had 2 failures for JITServer, unrelated to this PR

@dsouzai
Copy link
Contributor

dsouzai commented Aug 22, 2024

Win not running because there's no nodes online, merging.

@dsouzai dsouzai merged commit 8ebba54 into eclipse-openj9:master Aug 22, 2024
5 of 8 checks passed
@tajila
Copy link
Contributor

tajila commented Aug 22, 2024

These changes will need to be quad-delievered to 0.46.1, 0.47.0 and 0.48

@mpirvu
Copy link
Contributor Author

mpirvu commented Aug 22, 2024

I will wait one day for nightly testing to do its thing and tomorrow I'll start delivering this changes to the other branches.

@tajila
Copy link
Contributor

tajila commented Aug 22, 2024

Please tag me on the 0.46.1 PR

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.

3 participants