Skip to content

Skip HCR guards for inlining intrinsifiable methods #19469

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
May 10, 2024

Conversation

nbhuiyan
Copy link
Member

@nbhuiyan nbhuiyan commented May 8, 2024

For callees annotated with @IntrinsicCandidate, we shoud ignore any later redefinition as these methods are meant to have special handling for performance reasons. This annotation is only used in JCL code.

For callees annotated with @IntrinsicCandidate, we shoud ignore
any later redefinition as these methods are meant to have
special handling for performance reasons. This annotation is
only used in JCL code.

Signed-off-by: Nazim Bhuiyan <[email protected]>
@nbhuiyan
Copy link
Member Author

nbhuiyan commented May 9, 2024

@vijaysun-omr, @hzongaro I'd appreciate a review from either one of you.

@IntrinsicCandidate methods are currently treated as always worth inlining if no special handling for them is implemented.

In #18794, we had some methods annotated with @IntrinsicCandidate get special handling where they are replaced with simple loads (and no HCR guards), making later redefinition of such methods impossible. I am not sure whether it is possible to redefine intrinsifiable methods in Reference Implementation, so I will test that out.

@hzongaro
Copy link
Member

hzongaro commented May 9, 2024

I've found an issue that was reported against OpenJDK in 2013 — https://bugs.openjdk.org/browse/JDK-8013579 — that seems to discuss the question of whether a JVM must take into account the possibility that an @IntrinsicCandidate method might be redefined. The issue is still unresolved, so I think that means it would be safe for the OpenJ9 compiler to skip HCR guards for any "intrinsifiable" method, barring some future decision to the contrary.

Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

I think this change looks good. Thanks!

@hzongaro hzongaro self-assigned this May 9, 2024
@vijaysun-omr
Copy link
Contributor

Thanks @hzongaro for your thoroughness in looking that up, much appreciated

@hzongaro
Copy link
Member

hzongaro commented May 9, 2024

Jenkins test sanity,sanity.openjdk zlinux,alinux64,aix,win,win32 jdk8,jdk11,jdk17,jdk21

@vijaysun-omr
Copy link
Contributor

I approved but will let Henry merge it after testing etc.

@nbhuiyan
Copy link
Member Author

nbhuiyan commented May 9, 2024

5X Grinder of the failed test target jdk_security4_1 seen in the jdk11 aarch64 test job reproduces 1/5 times. Not sure if this failure is related to this change.

@hzongaro
Copy link
Member

hzongaro commented May 9, 2024

5X Grinder of the failed test target jdk_security4_1 seen in the jdk11 aarch64 test job reproduces 1/5 times. Not sure if this failure is related to this change.

I don't think it's related. I saw 2/5 failures in a Grinder run using a nightly build without your change.

@hzongaro
Copy link
Member

19:46:53  C:\Users\jenkins\workspace\Test_openjdk21_j9_sanity.openjdk_x86-64_windows_Personal_testList_2\aqa-tests\openjdk\openjdk-jdk\test\jdk\jdk\internal\util\ReferencedKeyTest.java:66: error: cannot find symbol
19:46:53      static void mapTest(boolean isSoft, Supplier<Map<ReferenceKey<Long>, String>> supplier) {
19:46:53                                                       ^
19:46:53    symbol:   class ReferenceKey
19:46:53    location: class ReferencedKeyTest

All test failures are unrelated to this change. Merging.

@hzongaro hzongaro merged commit 9a1f5b3 into eclipse-openj9:master May 10, 2024
@nbhuiyan nbhuiyan deleted the intrinsic-hcr branch April 28, 2025 18:58
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