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.

Sign up for GitHub

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

Jump to bottom

Methods to access class depth and flags #19473

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Methods to access class depth and flags #19473

wants to merge 2 commits into from

Conversation

hzongaro
Copy link
Member

@hzongaro hzongaro commented May 9, 2024

There are several places in the JIT compiler that generate IL to access the classDepthAndFlags field of j9class. This introduces methods in TR_J9VMBase to do that, as has previously been done with some methods that generate IL to access the classFlags field.

This change also deals with some typos in methods that refer to classDepthAndFlags incorrectly as classAndDepthFlags. One instance, J9::SymbolReferenceTable::findOrCreateClassAndDepthFlagsSymbolRef, will remain while it's still in use upstream in J9_PROJECT_SPECIFIC code in OMR.

Depends on OMR pull request eclipse/omr#7330

@hzongaro
Correct typos mentioning classDepthAndFlags field
f5bf9ef 
Several methods in the JIT compiler use the name of the
classDepthAndFlags field of J9Class in their names, but they incorrectly
refer to it as classAndDepthFlags.  This commit corrects the
typographical errors in the naming of those methods, but keeps one
instance that is used upstream in J9_PROJECT_SPECIFIC code in OMR until
that can be removed.
@hzongaro
Define methods to generate IL for accessing classDepthAndFlags field
8673620 
Various places in the compiler need to generate IL to load the
classDepthAndFlags field of j9class.  Those locations need to take into
account whether the target platform is 32-bit or 64-bit, as it affects
the width of the field.  Introducing methods in TR_J9VMBase to load
the field and potentially test the settings of bits in it hides some of
that.
@hzongaro hzongaro requested a review from 0xdaryl May 9, 2024 17:44
@hzongaro hzongaro mentioned this pull request May 10, 2024
Draft
@hzongaro
Copy link
Member Author

The upstream OMR pull request eclipse/omr#7330 has been promoted to openj9-omr, so this pull request is ready for review. @0xdaryl, may I ask you to review this pull request?

This pull request should be considered in conjunction with the upstream draft pull request eclipse/omr#7334, as that pull request uses the new TR_J9VMBase::testIsClassArrayType method.

One thing I want to point out is that TR_J9VMBase::testIsClassArrayType will produce IL that masks the relevant flags field to yield a non-zero int32 value if the class is an array type, or zero if it is not an array type. That's consistent with the existing testIsClassValueType, testIsClassPrimitiveValueType and testIsClassIdentityType methods in TR_J9VMBase. However, the places where the IL produced by testIsClassArrayType is used sometimes generate IL to compare the result to zero, compare it to the value of the mask, or right-shift it an appropriate amount to yield a result of zero or one.

The question is whether it's worthwhile continuing to use the result in all three of those ways, or whether all the places that use the result of that IL should just test whether it's equal to zero. If we preserve all three, it would probably make sense to hide those possibilities inside testIsClassArrayType itself, rather than having the caller in the second and third cases worry about what mask value it should use. And if we preserve all three, it would make sense to update testIsClassValueType, testIsClassPrimitiveValueType and testIsClassIdentityType to allow for the same possibilities for consistency.

@hzongaro hzongaro marked this pull request as ready for review May 10, 2024 20:38
@hzongaro hzongaro requested a review from dsouzai as a code owner May 10, 2024 20:38
@hzongaro hzongaro marked this pull request as draft May 14, 2024 17:41
@hzongaro
Copy link
Member Author

I'm moving this change to draft status as I'm going to introduce a change to Value Propagation that will take advantage of the new TR_J9VMBase::testIsClassArrayType method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xdaryl 0xdaryl Awaiting requested review from 0xdaryl

@dsouzai dsouzai Awaiting requested review from dsouzai dsouzai is a code owner

Assignees
No one assigned
Labels
comp:jit depends:omr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@hzongaro