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

Implement SVM J2I thunk validation and guarantee resolved virtual dispatch under SVM on x86 #14542

Merged
merged 3 commits into from
Mar 2, 2022

Conversation

jdmpapin
Copy link
Contributor

@jdmpapin jdmpapin commented Feb 17, 2022

The J2I thunk from method validation ensures that the J2I thunk appropriate to the method specified by the given method ID exists and is registered with the VM, and uses the address of the J2I thunk to define the given thunk ID.

Then with this validation, it's very straightforward to guarantee resolved virtual dispatch under SVM on x86. This will allow the IL generator to generate a resolved virtual call node for invokeinterface when calling a non-final method of Object, e.g. toString().

This PR requires eclipse/omr#6358 to be merged first

@jdmpapin
Copy link
Contributor Author

jdmpapin commented Feb 17, 2022

It's likely that corresponding changes should be just as straightforward in other code generators, but those changes are not included in this PR

@dsouzai dsouzai self-assigned this Feb 17, 2022
@dsouzai dsouzai added the depends:omr Pull request is dependent on a corresponding change in OMR label Feb 17, 2022
Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

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

Overall LGTM. However, there is one change requested regarding the pthread_jit_write_protect_np calls that are very likely unnecessary or even potentially problematic.

runtime/compiler/runtime/RelocationRecord.cpp Outdated Show resolved Hide resolved
@dsouzai
Copy link
Contributor

dsouzai commented Feb 17, 2022

Oh also, you need to update the minor version for JITServer, as a new relocation record has been added (see https://github.com/eclipse-openj9/openj9/blob/master/doc/compiler/misc/BestPractices.md#jitserver)

@jdmpapin
Copy link
Contributor Author

Removed the pthread_jit_write_protect_np() calls and bumped JITServer's MINOR_NUMBER

This makes the J2I thunk loading and registration logic available to be
called for the J2I thunk from method SVM validation that will be defined
in a subsequent commit.

The calls to pthread_jit_write_protect_np() are removed because although
this method is writing to the code cache, the write protection is
already handled in applyRelocationAtAllOffsets().
This validation ensures that the J2I thunk appropriate to the method
specified by the given method ID exists and is registered with the VM,
and uses the address of the J2I thunk to define the given thunk ID.
This will allow the IL generator to generate a resolved virtual call
node for invokeinterface when calling a non-final method of Object, e.g.
toString().
@jdmpapin
Copy link
Contributor Author

Rebased to fix the MINOR_NUMBER conflict

@dsouzai
Copy link
Contributor

dsouzai commented Feb 28, 2022

jenkins test sanity.functional+aot xlinux jdk17

@dsouzai
Copy link
Contributor

dsouzai commented Feb 28, 2022

jenkins test sanity all jdk17

1 similar comment
@dsouzai
Copy link
Contributor

dsouzai commented Mar 1, 2022

jenkins test sanity all jdk17

@dsouzai
Copy link
Contributor

dsouzai commented Mar 1, 2022

s390x failure because of #13128

win64 failure seems to be infra related? That was the old win job I guess.

@dsouzai
Copy link
Contributor

dsouzai commented Mar 2, 2022

jenkins test sanity.functional+aot xlinux jdk17

@dsouzai
Copy link
Contributor

dsouzai commented Mar 2, 2022

The AOT build was aborted because I guess it took too long to get scheduled/run -_-

@dsouzai
Copy link
Contributor

dsouzai commented Mar 2, 2022

jenkins test sanity.functional+aot xlinux jdk17

@dsouzai dsouzai merged commit f99336c into eclipse-openj9:master Mar 2, 2022
@jdmpapin
Copy link
Contributor Author

jdmpapin commented Mar 2, 2022

@IBMJimmyk, since we discussed doing the same change on Power, see the final (x86) commit in this PR. If Power code gen already only generates a plain call through the vtable for resolved virtual calls, it should be similar

@joransiu, I haven't discussed this with you, but a similar statement applies to z in case you want to improve code generation for this case in SVM-based AOT

@knn-k
Copy link
Contributor

knn-k commented Mar 3, 2022

I opened #14638 to fix build failures on AArch64 macOS.
You need to call pthread_jit_write_protect_np() before and after writing to the JIT code cache.

knn-k added a commit to knn-k/openj9 that referenced this pull request Mar 4, 2022
This commit adds calls to pthread_jit_write_protect_np() for AArch64
macOS, which were removed by eclipse-openj9#14542.

Signed-off-by: KONNO Kazuhiro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jit:aot comp:jit depends:omr Pull request is dependent on a corresponding change in OMR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants