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

Add SVM AOT support for resolved invokeHandle/invokeDynamic dispatch #20373

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dsouzai
Copy link
Contributor

@dsouzai dsouzai commented Oct 17, 2024

  • Add validation for TR_ValidateDynamicMethodFromCallsiteIndex
  • Add validation for TR_ValidateHandleMethodFromCPIndex
  • Add relocations for TR_CallsiteTableEntryAddress
  • Add relocations for TR_MethodTypeTableEntryAddress
  • Conditionally support storing LambdaForms into the SCC if -Xshareclasses:shareLambdaForm is enabled.

Depends on eclipse/omr#7494

Add new TR_RelocationErrorCode kinds to indicate validation failures for
TR_ValidateDynamicMethodFromCallsiteIndex and
TR_ValidateHandleMethodFromCPIndex.

Signed-off-by: Irwin D'Souza <[email protected]>
@dsouzai dsouzai added comp:jit comp:jit:aot project:MH Used to track Method Handles related work depends:omr Pull request is dependent on a corresponding change in OMR labels Oct 17, 2024
@dsouzai
Copy link
Contributor Author

dsouzai commented Oct 17, 2024

@hangshao0 does f9c7eba look like the proper way to conditionally support storing LambdaForms into the SCC? I wanted something like this so that we can add AOT MH tests without having to enable by default both LambdaForms in the SCC & Resolved invokeHandle/invokeDynamic dispatch in AOT code.

@hangshao0
Copy link
Contributor

does f9c7eba look like the proper way to conditionally support storing LambdaForms into the SCC?

Looks mostly fine. But you may want to set J9_FINDCLASS_FLAG_LAMBDAFORM | J9_FINDCLASS_FLAG_DO_NOT_SHARE in the case shareLambdaForm is off. Set flag J9_FINDCLASS_FLAG_LAMBDAFORM in the case shareLambdaForm is on.

@dsouzai dsouzai marked this pull request as ready for review October 17, 2024 19:20
@dsouzai
Copy link
Contributor Author

dsouzai commented Oct 17, 2024

@jdmpapin could you please review?

@hangshao0 could you please review the VM changes?

@hangshao0
Copy link
Contributor

For now, is the new option shareLambdaForm a public option for everyone or a private option just to be used by OpenJ9 developers/builds ?

I remember the reason of not storing lambdaForms class is that it has a performance issue. Storing lambdaForms requires the VM to do extra class comparisons before we can return the correct lamdaForm class. There is a PR #19763 that could address the performance issue. But it is not merged so the performance issue is still there.

@dsouzai
Copy link
Contributor Author

dsouzai commented Oct 17, 2024

For now, is the new option shareLambdaForm a public option for everyone or a private option just to be used by OpenJ9 developers/builds ?

I'm treating this as an internal option for devs/testing. It can be deleted in the future when LambdaForms are stored into the SCC.

Is there a better way to create/use an option for dev/testing purposes?

@hangshao0
Copy link
Contributor

I'm treating this as an internal option for devs/testing. It can be deleted in the future when LambdaForms are stored into the SCC.
Is there a better way to create/use an option for dev/testing purposes?

The reason I was asking is because public option needs to be documented in the user guide, but since it it private option, we don't need documentation change. The way you are doing it here is fine.

runtime/bcutil/ROMClassBuilder.cpp Outdated Show resolved Hide resolved
runtime/shared_common/shrinit.h Outdated Show resolved Hide resolved
Copy link
Contributor

@hangshao0 hangshao0 left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jit:aot comp:jit comp:vm depends:omr Pull request is dependent on a corresponding change in OMR project:MH Used to track Method Handles related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants