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

Set min jump table entries when compiling interpreter #20908

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

midronij
Copy link
Contributor

OpenXL on PPC has been known to have some issues with branch tables/indirect branch prediction before, which, when compiling OpenJ9 on AIX with OpenXL, resulted in a ~15% performance regression from before (i.e.: when building with xlC). To eliminate this degredation, the compiler flags -mllvm -ppc-min-jump-table-entries=512 can be used on the souce files containing the interpreter loop.

OpenXL on PPC has been known to have some issues with branch
tables/indirect branch prediction before, which, when compiling
OpenJ9 on AIX with OpenXL, resulted in a ~15% performance regression
from before (i.e.: when building with xlC). To eliminate this
degredation, the compiler flags "-mllvm -ppc-min-jump-table-entries=512"
can be used on the souce files containing the interpreter loop.

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

@zl-wang @pshipton when you have a moment could you please review? The change is very small so it shouldn't take too long

@zl-wang
Copy link
Contributor

zl-wang commented Jan 10, 2025

Looks like: with this change, variable CMAKE_C_COMPILER_IS_OPENXL is needed for cmake after all (I have impression that Keith requested it to be removed from cmake).

Copy link
Contributor

@zl-wang zl-wang left a comment

Choose a reason for hiding this comment

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

LGTM

@pshipton pshipton merged commit 610e81e into eclipse-openj9:master Jan 10, 2025
2 checks passed
@midronij
Copy link
Contributor Author

Looks like: with this change, variable CMAKE_C_COMPILER_IS_OPENXL is needed for cmake after all (I have impression that Keith requested it to be removed from cmake).

I believe the one Keith wanted to remove was actually OMR_ENV_OPENXL. I will be opening a PR for that change (as well as some others that Keith requested) soon

@keithc-ca
Copy link
Contributor

Looks like: with this change, variable CMAKE_C_COMPILER_IS_OPENXL is needed for cmake after all (I have impression that Keith requested it to be removed from cmake).

No, I requested that OMR_ENV_OPENXL not be added to the environment (it serves no purpose in cmake builds).

However, CMAKE_C_COMPILER_IS_OPENXL doesn't seem to fit the existing convention of using OMR_TOOLCONFIG to distingush toolchains. In fact, it appears to identify specific flavours of gnu or xlc, neither of which seem appropriate to me.

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