Skip to content

Disable TLH prefetching for portable AOT code - #20027

Merged
dsouzai merged 1 commit into
eclipse-openj9:masterfrom
mpirvu:tlhprefetch
Aug 22, 2024
Merged

Disable TLH prefetching for portable AOT code#20027
dsouzai merged 1 commit into
eclipse-openj9:masterfrom
mpirvu:tlhprefetch

Conversation

@mpirvu

@mpirvu mpirvu commented Aug 20, 2024

Copy link
Copy Markdown
Contributor

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.

@dsouzai dsouzai left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread runtime/compiler/control/J9Options.cpp Outdated
@dsouzai dsouzai self-assigned this Aug 21, 2024
@mpirvu

mpirvu commented Aug 21, 2024

Copy link
Copy Markdown
Contributor Author

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

mpirvu commented Aug 21, 2024

Copy link
Copy Markdown
Contributor Author

jenkins test sanity all jdk17

@dsouzai

dsouzai commented Aug 21, 2024

Copy link
Copy Markdown
Contributor
[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

mpirvu commented Aug 21, 2024

Copy link
Copy Markdown
Contributor Author

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

mpirvu commented Aug 21, 2024

Copy link
Copy Markdown
Contributor Author

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 <mpirvu@ca.ibm.com>
@mpirvu

mpirvu commented Aug 22, 2024

Copy link
Copy Markdown
Contributor Author

@mpirvu

mpirvu commented Aug 22, 2024

Copy link
Copy Markdown
Contributor Author

jenkins test sanity amac,alinux,win64 jdk17

@mpirvu

mpirvu commented Aug 22, 2024

Copy link
Copy Markdown
Contributor Author

jenkins test sanity amac,alinux,win jdk17

@mpirvu

mpirvu commented Aug 22, 2024

Copy link
Copy Markdown
Contributor Author

alinux had 2 failures for JITServer, unrelated to this PR

@dsouzai

dsouzai commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

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

@dsouzai
dsouzai merged commit 8ebba54 into eclipse-openj9:master Aug 22, 2024
@tajila

tajila commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

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

@mpirvu

mpirvu commented Aug 22, 2024

Copy link
Copy Markdown
Contributor Author

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

tajila commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

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