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 boxing/unboxing during VectorAPIExpansion under an option #20685

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

gita-omr
Copy link
Contributor

  • introduce new category of temps: vec_len_boxed_unknown
  • build temp and whole webs as before
  • wherever a temp was invalidated before assign vec_len_boxed_unknown instead
  • store vectorization info for calls per node vs. per symbol reference for temps
  • always vectorize nodes that are calls to vector API methods, even if input vectors need to be unboxed or result needs to be boxed
  • since boxing/unboxing is not supported for all types, have an extra pass just before transformation and invalidate whole web if it contains unsupported boxing/unboxing
  • transformation happens as before, except that we first check if children of non-vectorized node need to be boxed or children of vectorized node need to be unboxed, insert boxing/unboxing correspondingly
  • this prototype is only enabled under -Xjit:enableVectorAPIBoxing option and the default behaviour shoulld be preserved

@gita-omr gita-omr changed the title Implement boxing/unboxing during VectorAPIExpansion under an option WIP: Implement boxing/unboxing during VectorAPIExpansion under an option Nov 27, 2024
@gita-omr
Copy link
Contributor Author

Noticed a bug..

@gita-omr
Copy link
Contributor Author

gita-omr commented Dec 2, 2024

Fixed some bugs and addressed second round of comments. openjdk sanity passes (with and without the option). However, I had to disable boxing of Mask objects due to a failure in that bucket. Debugging now.

@gita-omr
Copy link
Contributor Author

gita-omr commented Dec 5, 2024

Added improvements and addressed latest comments except for the doxygen comments.

@gita-omr gita-omr changed the title WIP: Implement boxing/unboxing during VectorAPIExpansion under an option Implement boxing/unboxing during VectorAPIExpansion under an option Dec 5, 2024
@gita-omr
Copy link
Contributor Author

gita-omr commented Dec 5, 2024

Squashed and removed WIP.

- introduce new category of temps: vec_len_boxed_unknown
- build temp and whole webs as before
- wherever a temp was invalidated before assign vec_len_boxed_unknown instead
- store vectorization info for calls per node vs. per symbol reference for temps
- always vectorize nodes that are calls to vector API methods, even if input vectors
  need to be unboxed or result needs to be boxed
- since boxing/unboxing is not supported for all types, have an extra pass just before
  transformation and invalidate whole web if it contains unsupported boxing/unboxing
- transformation happens as before, except that we first check if children of non-vectorized
  node need to be boxed or children of vectorized node need to be unboxed, insert
  boxing/unboxing correspondingly
- this prototype is only enabled under -Xjit:enableVectorAPIBoxing option and the default
  behaviour shoulld be preserved
@gita-omr
Copy link
Contributor Author

gita-omr commented Dec 5, 2024

Addressed latest comment.

@knn-k
Copy link
Contributor

knn-k commented Dec 5, 2024

Jenkins test sanity alinux64,plinux,xlinux,zlinux jdk21

@gita-omr
Copy link
Contributor Author

gita-omr commented Dec 5, 2024

s390 failures seem to be unrelated.

@knn-k
Copy link
Contributor

knn-k commented Dec 5, 2024

Jenkins test sanity alinux64,plinux,xlinux,zlinux jdk21

@knn-k
Copy link
Contributor

knn-k commented Dec 6, 2024

A test job on xlinux failed with the message below:
https://openj9-jenkins.osuosl.org/job/Test_openjdk21_j9_sanity.functional_x86-64_linux_Personal_testList_1/189/consoleText

[2024-12-06T02:27:21.089Z] Boooo - there are rogue processes kicking about
[2024-12-06T02:27:21.089Z] Issuing a kill to all processes shown above

@knn-k
Copy link
Contributor

knn-k commented Dec 6, 2024

Jenkins test sanity.functional xlinux jdk21

@knn-k
Copy link
Contributor

knn-k commented Dec 6, 2024

Test failures on plinux and zlinux are related to CRIU. They don't seem to be caused by this PR. Waiting for xlinux results.

plinux:
cmdLineTester_criu_jitserverAcrossCheckpoint_0
https://openj9-jenkins.osuosl.org/job/Test_openjdk21_j9_sanity.functional_ppc64le_linux_Personal_testList_0/127/consoleText

zlinux:
cmdLineTester_criu_jitPostRestore_3
https://openj9-jenkins.osuosl.org/job/Test_openjdk21_j9_sanity.functional_s390x_linux_Personal_testList_1/269/consoleText

@knn-k knn-k self-assigned this Dec 6, 2024
@knn-k knn-k merged commit 2aa906a into eclipse-openj9:master Dec 6, 2024
13 of 15 checks passed
switch (type)
{
case TR::Float:
j9class = vm->floatArrayClass;
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: Accessing J9JavaVM directly in the optimizer will break JITServer functionality. Typically a frontend function needs to be created to fetch the appropriate field and that function needs to be overridden at the server frontend. PR #20592 shows an example

newObject->setSymbolReference(comp()->getSymRefTab()->findOrCreateNewObjectSymbolRef(comp()->getMethodSymbol()));

TR_J9VMBase *fej9 = comp()->fej9();
TR::VMAccessCriticalSection getClassFromSignature(fej9);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this VMaccessCriticalSection needed here? Typically such sections access objects and we want to prevent GC moving those objects around while we inspect them. Such pieces of code cannot be executed at the server; the server will send a message to the client and ask it to do the work on its behalf.

if (_trace)
traceMsg(comp(), "Boxed: %dth child of node %p into %p\n", i, node, newObject);

if (TR::Options::getVerboseOption(TR_VerboseVectorAPI))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer the optimizer to use compilation logs instead of vlog. Vlog is typically used for runtime. Verbose options are global and they are not passed to the JITServer If the method is compiled remotely, these messages will not appear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants