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

Override isCompilable for TR_ResolveRelocatableJ9Method #16556

Merged
merged 2 commits into from
Jan 20, 2023

Conversation

nbhuiyan
Copy link
Member

@nbhuiyan nbhuiyan commented Jan 13, 2023

TR_ResolvedRelocatableJ9Method::isCompilable should match what TR::CompilationInfoPerThreadBase::isMethodIneligibleForAot returns for a method. This override is necessary because methods in java/lang/invoke/* are marked ineligible from AOT compilations, and therefore isCompilable should also return false for these methods during AOT. This impacts what methods are considered for inlining.

Fixes: #16161, #14135

Signed-off-by: Nazim Bhuiyan [email protected]

@nbhuiyan
Copy link
Member Author

@dsouzai @0xdaryl FYI

@ChengJin01
Copy link

FYI: @tajila, @pshipton, @fengxue-IS

@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 16, 2023

Given the required symmetry between TR_ResolvedRelocatableJ9Method::isCompilable() and TR::CompilationInfoPerThreadBase::isMethodIneligibleForAot() can those functions share common logic so that this kind of problem doesn't occur in the future?

Even as it stands right now your new isCompilable() function does not include the check for isVMDeepCopySupported(). I have a suspicion that method is no longer of interest for OpenJ9 but that should be confirmed. If it is of interest then you should include it, otherwise it should be cleaned up from isMethodIneligibleForAot().

@nbhuiyan
Copy link
Member Author

@dsouzai Requesting your input on whether isVMDeepCopySupported still needs to be checked in isMethodIneligibleForAot.

@dsouzai
Copy link
Contributor

dsouzai commented Jan 16, 2023

I think if isVMDeepCopySupported is a query that's still used, it is very likely that atm AOT won't know how to deal with it.

Also updated existing use of the function.

Signed-off-by: Nazim Bhuiyan <[email protected]>
TR_ResolvedRelocatableJ9Method::isCompilable should agree with
what TR::CompilationInfo::isMethodIneligibleForAot
returns for a method. This override is necessary because methods
in java/lang/invoke/* are ineligible for AOT compilations,
and therefore isCompilable should also return false for these
methods during AOT. This impacts what methods are considered for
inlining.

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

@0xdaryl based on Irwin's comment, I have not made any changes to the check for isVMDeepCopySupported. I changed the implementation of isCompilable based on your feedback such that no dual list of method(s) to exclude for AOT are needed. This PR is now ready for review.

@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 17, 2023

Jenkins test sanity all jdk17,jdk19

@0xdaryl 0xdaryl self-assigned this Jan 17, 2023
@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 18, 2023

Jenkins test sanity xlinux jdk17

@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 18, 2023

Jenkins test sanity plinux,aix,xmac,amac,alinux64 jdk17,jdk19

@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 18, 2023

Jenkins test sanity xlinux jdk19

@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 19, 2023

Jenkins test sanity plinux,aix,xmac,amac,alinux64,xlinux jdk17,jdk19

@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 19, 2023

Jenkins test sanity aix jdk17

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.

OpenJDK java/foreign/TestDowncallScope.java NoSuchMethodError MethodHandle.invokeBasic
5 participants