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

Remove Q type checks in Valhalla code #18157

Open
hangshao0 opened this issue Sep 18, 2023 · 8 comments
Open

Remove Q type checks in Valhalla code #18157

hangshao0 opened this issue Sep 18, 2023 · 8 comments
Labels
comp:vm project:valhalla Used to track Project Valhalla related work

Comments

@hangshao0
Copy link
Contributor

Currently many places in the VM rely on Q type check for the null-restricted value type. The Q type is going to be removed in the future. L type will be used for all value types, no matter it is null-restricted or not. For the fields, the Q type is going to be replaced with null-restricted attribute,

Also the keyword "primitive" is going to be removed.

The null restricted fields will be treated the same as Q type fields. For now, we need to add null-restricted attribute checks into the places where we are doing Q type field checks. After javac is updated, we can remove the Q type checks completely.

Some of our testing code is generating Q types and using "primitive", they need to be updated as well.

@hangshao0 hangshao0 added comp:vm project:valhalla Used to track Project Valhalla related work labels Sep 18, 2023
@hangshao0
Copy link
Contributor Author

hangshao0 commented Sep 18, 2023

Also the keyword "primitive" is going to be removed.

When "primitive" is removed, J9ROMCLASS_IS_PRIMITIVE_VALUE_TYPE() will always be false. With the new javac, we can use J9ROMCLASS_IS_VALUE() instead.

Since only null-restricted field can be potentially flattened, we can add a check of null restricted field (J9FieldFlagIsNullRestricted) in J9_IS_FIELD_FLATTENED().

@dmitripivkine
Copy link
Contributor

I am not sure right away, but I can check what is used by GC for recognition of VT "leaf object" (does not introduce new object references). We are going to need the way to make "leaf object" detection for VT objects in order to have GC optimization running.

@hangshao0
Copy link
Contributor Author

Also the keyword "primitive" is going to be removed.

When "primitive" is removed, J9ROMCLASS_IS_PRIMITIVE_VALUE_TYPE() will always be false. With the new javac, we can use J9ROMCLASS_IS_VALUE() instead.

Not sure about the "we can use J9ROMCLASS_IS_VALUE() instead" part. There is a ACC_NON_ATOMIC flag introduced in the ImplicitCreation Attribute, do you know if that is introduced to the replace "primitive" keyword ? @TobiAjila

@theresa-m
Copy link
Contributor

theresa-m commented Oct 16, 2023

Before removing Q type checks I'm modifying existing Valhalla functional tests to be compatible as much as possible with the lw5 compiler.

edit: tests in src_lw5 compile with the latest version of lw5. Additional vm support is needed for them to pass.

No changes are needed for these files, they will remain in src:

  • ValueTypeOptTests
  • ValhallaAttributeTests
  • ValhallaAttributeGenerator

These tests have been split into src_qtypes and src_lw5 and compile with lw5 commit dc715159feda32945d5c2351ede9c6be04a64d6f:

hangshao0 added a commit to hangshao0/openj9 that referenced this issue Oct 25, 2023
Q type is going to be removed. Instead we should do null restricted
modifier check. 
In order for existing test to work, this change does not remove q type
checks from the existing code. NULL restricted check is appended to
places where Q type check is done.

issue eclipse-openj9#18157

Signed-off-by: Hang Shao <[email protected]>
@theresa-m
Copy link
Contributor

theresa-m commented Feb 6, 2024

This is a new list of some of the vm tasks to be done once javac is updated for null restricted types. Aside from removing qtype and primitive references we will also need to:

  • Enable null restricted (lw5) tests, remove qtype tests, and merge any vm changes that are not compatible in the current code base. These are being tracked here Valhalla: test updates for lw5 #18558

Edit: given that q is removed in the current javac and null restricted support is not yet there tests in src_qtypes have been updated and no longer have dependencies on q.

This pr can be closed once all references to q and primitive are removed.

@theresa-m
Copy link
Contributor

#19799 will remove all remaining uses of 'q' and 'qtype' from OpenJ9. @a7ehuo is going to look at removing any q references from OMR.

The next step to close out this issue will be removing references to the old primitive flag for value types.

@a7ehuo
Copy link
Contributor

a7ehuo commented Jul 3, 2024

@a7ehuo is going to look at removing any q references from OMR.

Opened eclipse/omr#7399

@theresa-m
Copy link
Contributor

theresa-m commented Sep 18, 2024

The following q and primitive reference are still outstanding before this issue can be closed:

ReferenceType naming

J9_IS_J9CLASS_PRIMITIVE_VALUETYPE

J9::ClassEnv::isPrimitiveValueTypeClass

  • remaining uses are in compiler code

TR_J9VMBase::checkArrayCompClassPrimitiveValueType

  • Remaining uses in compiler code

J9ClassIsPrimitiveValueType flag

fyi @a7ehuo I'm sure you're already aware of these to be removed when it makes sense to do so, just wanted to summarize the remaining work that I'm aware of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:vm project:valhalla Used to track Project Valhalla related work
Projects
Status: TODO: VM
Development

No branches or pull requests

4 participants