valuetypeddrtests with Valhalla lw5 #18597

theresa-m · 2023-12-11T21:57:01Z

Create copy flattenedvaluetypeddrtests
Modify valuetypeddrtests to work with lw5
Add checks for lw5 attributes to ddr and vm

The lw5 tests can be run manually by including changes from #18558 and using a build of https://github.com/openjdk/valhalla/tree/lw5 as TEST_JDK_HOME while running make compile

hangshao0 · 2023-12-12T16:46:06Z

These tests will be modified in a future change set to be compatible with the lw5 compiler

I see this is marked as a draft. Are you planning to add the future changes into this PR ?

theresa-m · 2023-12-12T20:53:31Z

Yea... I changed my mind. I'll update the comment.

Signed-off-by: Theresa Mammarella <[email protected]>

theresa-m · 2023-12-14T15:49:03Z

@hangshao0 this is ready for review. Only flattenedvaluetypeddrtests.xml really needs copying because the test classes in DDRValueTypeTest change.

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/j9/ObjectFieldInfo.java

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/j9/J9ObjectFieldOffsetIterator_V1.java

test/functional/cmdLineTests/valuetypeddrtests/flattened32bitRefBackfillTest.xml

test/functional/cmdLineTests/valuetypeddrtests/flattenedvaluetypeddrtests_lw5.xml

hangshao0 · 2024-01-04T15:25:39Z

Could you squash the commits ?

Signed-off-by: Theresa Mammarella <[email protected]>

theresa-m · 2024-01-04T20:06:02Z

They are squashed now and I applied your feedback.

hangshao0 · 2024-01-04T20:38:29Z

Jenkins test sanity,extended winval jdknext

hangshao0 · 2024-01-04T20:38:41Z

Jenkins test sanity,extended aixval jdknext

theresa-m · 2024-01-05T15:43:32Z

I am working on the test failure in ValhallaAttributeTests.

theresa-m · 2024-01-05T18:31:53Z

The failure happens because J9ClassIsPrimitiveValueType is being set for classes that are either primitive (qtypes) or have an implicit creation attribute with default flag (lw5) due to the changes in createramclass.cpp... which is obviously wrong but I was hoping to deal with later :)

The JIT code creates signatures with q's based on that flag such as https://github.com/eclipse-openj9/openj9/blob/master/runtime/compiler/env/VMJ9.cpp#L2539 which triggers the test failure.

To solve this I think the J9ClassIsPrimitiveValueType should remain just for primitive classes and the case for classes with the implicit creation attribute + default flag can have its own flag set here https://github.com/eclipse-openj9/openj9/blob/master/runtime/vm/createramclass.cpp#L2351. This flag will also be used in the future for setting default values to value classes.

What do you think @hangshao0 ?

hangshao0 · 2024-01-05T22:09:42Z

The failure happens because J9ClassIsPrimitiveValueType is being set for classes that are either primitive (qtypes) or have an implicit creation attribute with default flag (lw5) due to the changes in createramclass.cpp... which is obviously wrong but I was hoping to deal with later :)

Strictly speaking, a class with implicit creation attribute + default flag is not fully equivalent to primitive VT (null restricted VT). I guess you are adding J9ClassIsPrimitiveValueType change here for now to pass some more tests ?

The JIT code creates signatures with q's based on that flag such as https://github.com/eclipse-openj9/openj9/blob/master/runtime/compiler/env/VMJ9.cpp#L2539 which triggers the test failure.

Does that mean the test can pass if we turn off JIT with -Xint ?

Not sure if changing this check to !J9_IS_J9CLASS_PRIMITIVE_VALUETYPE(valueClass) or relaxing it to !J9ROMCLASS_IS_VALUE(valueROMClass) helps to pass this test.

hzongaro · 2024-01-08T13:54:23Z

The JIT code creates signatures with q's based on that flag such as https://github.com/eclipse-openj9/openj9/blob/master/runtime/compiler/env/VMJ9.cpp#L2539 which triggers the test failure.

One of the necessary conditions the JIT is checking in deciding whether to use a Q in the signature is TR::Compiler->om.isQDescriptorForValueTypesSupported(). The implementation of that method only checks whether support for flattenable/null-restricted value types are enabled - it doesn't distinguish whether Q is permitted as part of a signature. Do we have something that the JIT could use to distinguish support for null-restricted types from whether Q is used in the signature of those types?

theresa-m · 2024-01-08T15:28:18Z

Not sure if changing this check to !J9_IS_J9CLASS_PRIMITIVE_VALUETYPE(valueClass) or relaxing it to !J9ROMCLASS_IS_VALUE(valueROMClass) helps to pass this test.

I could try that but was thinking it would be better for testing purposes not to rely on the Q case code if possible.

Strictly speaking, a class with implicit creation attribute + default flag is not fully equivalent to primitive VT (null restricted VT). I guess you are adding J9ClassIsPrimitiveValueType change here for now to pass some more tests ?

You're right that they are not the same. The way I was thinking about it is primitive has the behaviours that translate into the nullrestricted world roughly as:

eligible for flattening --> from a class point of view (since the field in question is often not available in the vm code) having an implicitcreation attribute with a default flag may mean its eligible for flattening but later the fields nullrestricted flag will need to be checked as well
not nullable/needs a default value --> nullrestricted flag
atomicity --> triggered by implicitcreation attribute + non_atomic flag

In this task I'm thinking about only the first one. The other two are not fully supported and tested for the nullrestricted world. So while having a flag for implicitcreation attribute + default isn't really a replacement for the primitive flag it is in needed to accomplish the "eligible for flattening" aspect which I think is a good start. That said I'm not very familiar with the compiler code so not sure if I'm missing something obvious there.

Do we have something that the JIT could use to distinguish support for null-restricted types from whether Q is used in the signature of those types?
So far there is no way to tell if nullrestricted is enabled at runtime, only the flag for javac '-XDenableNullRestrictedTypes'

J9ClassIsPrimitiveValueType is being set in ram classes for implicitcreation fields to help with flattening. Prevent errors until a new flag for implicitcreation/default is added. Signed-off-by: Theresa Mammarella <[email protected]>

theresa-m · 2024-01-09T15:02:57Z

After talking to Hang I have added his suggestion to relax this check to complete this pull request. A new ram class flag for implicitcreation/default classes will be added in a future change set.

hangshao0 · 2024-01-09T15:47:21Z

Jenkins test sanity,extended winval jdknext

hangshao0 · 2024-01-09T15:47:31Z

Jenkins test sanity,extended aixval jdknext

hangshao0 · 2024-01-09T19:37:50Z

Seems Valhalla build is now broken.

@JasonFengJ9 As #18710 is merged yesterday, I believe this commit ibmruntimes/openj9-openjdk-jdk22@2454954 needs to be ported to https://github.com/ibmruntimes/openj9-openjdk-jdk.valuetypes

JasonFengJ9 · 2024-01-09T21:48:04Z

Cherry-picked ibmruntimes/openj9-openjdk-jdk22@2454954, and verified at a local build.

@hangshao0 it is ready for the PR runs.

hangshao0 · 2024-01-09T22:06:15Z

Jenkins test sanity,extended winval jdknext

hangshao0 · 2024-01-09T22:06:24Z

Jenkins test sanity,extended aixval jdknext

hangshao0 · 2024-01-10T16:03:54Z

FYI @llxia, here is the PR builds that we are starting to see the asm/asm-all jar related failures.

https://openj9-jenkins.osuosl.org/job/Test_openjdkValhalla_j9_extended.functional_x86-64_windows_valhalla_Personal/24/
https://openj9-jenkins.osuosl.org/job/Test_openjdkValhalla_j9_extended.functional_ppc64_aix_valhalla_Personal/9/

19:20:26   [ERR] java.lang.NoClassDefFoundError: org.objectweb.asm.ClassWriter
19:20:26   [ERR] 	at org.openj9.test.lworld.ValueTypeGenerator.generateClass(ValueTypeGenerator.java:206)
19:20:26   [ERR] 	at org.openj9.test.lworld.ValueTypeGenerator.generateValueClass(ValueTypeGenerator.java:1236)
19:20:26   [ERR] 	at org.openj9.test.lworld.ValueTypeGenerator.generateValueClass(ValueTypeGenerator.java:1207)
19:20:26   [ERR] 	at org.openj9.test.lworld.ValueTypeTests.testCreateValueInt(ValueTypeTests.java:1344)
19:20:26   [ERR] 	at org.openj9.test.lworld.DDRValueTypeTest.createAndCheckValueType(DDRValueTypeTest.java:40)
19:20:26   [ERR] 	at org.openj9.test.lworld.DDRValueTypeTest.main(DDRValueTypeTest.java:32)
19:20:26   [ERR] Caused by: java.lang.ClassNotFoundException: org.objectweb.asm.ClassWriter
19:20:26   [ERR] 	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:827)
19:20:26   [ERR] 	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
19:20:26   [ERR] 	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:1104)
19:20:26   [ERR] 	... 6 more

It might happen on all other platforms.

llxia · 2024-01-10T21:42:19Z

re #18597 (comment), There are mismatches between the downloaded dir and the referenced dir. I will fix this.

related: eclipse-openj9#18597 (comment) Signed-off-by: Lan Xia <[email protected]>

hangshao0 · 2024-01-11T15:44:03Z

@theresa-m Are you looking at the sanity.functional test failures ?

theresa-m · 2024-01-11T16:05:13Z

I've been looking at the sanity failure (thanks @a7ehuo for helping me understand some of the compiler code!) and I as far as I can tell is an error in jit support.

The failure is for _ValhallaAttributeTests_4, _ValhallaAttributeTests_5, _ValhallaAttributeTests_6 and since this pull request is the first time for the nullrestricted world that fields are being flattened there is new code being executed.

This check I'm pretty sure is missing from the flattened case.

Since its indirectly related to this change set I plan to disable these tests for now until it can be fixed properly. Any objections?

hangshao0 · 2024-01-11T16:12:09Z

Since its indirectly related to this change set I plan to disable these tests for now until it can be fixed properly. Any objections?

I am fine with disabling them for now. An issue can be created to track it so that we don't forget to enable them once things are fixed.

theresa-m · 2024-01-11T16:35:22Z

The tests are disabled now.

test/functional/Valhalla/playlist.xml

Failing due to missing NPE check for pufield in the nullrestricted flattened case. Signed-off-by: Theresa Mammarella <[email protected]>

hangshao0 · 2024-01-11T19:18:39Z

Jenkins test sanity,extended winval jdknext

hangshao0 · 2024-01-11T19:18:49Z

Jenkins test sanity,extended aixval jdknext

theresa-m added comp:test project:valhalla Used to track Project Valhalla related work labels Dec 11, 2023

theresa-m marked this pull request as ready for review December 11, 2023 21:57

theresa-m force-pushed the split_ddrlw5 branch from 46e5c83 to 0074ceb Compare December 11, 2023 22:06

theresa-m marked this pull request as draft December 12, 2023 14:23

theresa-m changed the title ~~Create lw5 copy of valuetypeddrtests~~ valuetypeddrtests with Valhalla lw5 Dec 12, 2023

theresa-m force-pushed the split_ddrlw5 branch from 0074ceb to 8169b16 Compare December 12, 2023 21:06

theresa-m added 2 commits December 14, 2023 10:26

Add missing vm/ddr checks to flatten fields

cecf66b

Signed-off-by: Theresa Mammarella <[email protected]>

Create lw5 copy of flattenedvaluetypeddrtests

4836356

Signed-off-by: Theresa Mammarella <[email protected]>

theresa-m force-pushed the split_ddrlw5 branch from c616b2f to 8d9f9e9 Compare December 14, 2023 15:29

theresa-m marked this pull request as ready for review December 14, 2023 15:47

hangshao0 reviewed Dec 15, 2023

View reviewed changes

Update Valhalla ddr tests for lw5

797b130

Signed-off-by: Theresa Mammarella <[email protected]>

theresa-m force-pushed the split_ddrlw5 branch from b06901c to 797b130 Compare January 4, 2024 20:05

Relax Q type check for implicitcreation/default

a418dfb

J9ClassIsPrimitiveValueType is being set in ram classes for implicitcreation fields to help with flattening. Prevent errors until a new flag for implicitcreation/default is added. Signed-off-by: Theresa Mammarella <[email protected]>

theresa-m force-pushed the split_ddrlw5 branch from b8b025e to a418dfb Compare January 9, 2024 15:02

theresa-m requested a review from hangshao0 January 9, 2024 15:03

llxia mentioned this pull request Jan 10, 2024

Fix asm.jar path mismatch in valuetype tests #18736

Merged

llxia added a commit to llxia/openj9 that referenced this pull request Jan 10, 2024

Fix asm.jar path mismatch in valuetype tests

34ad1f9

related: eclipse-openj9#18597 (comment) Signed-off-by: Lan Xia <[email protected]>

theresa-m mentioned this pull request Jan 11, 2024

Fix and enable ValhallaAttributeTests variations 4/5/6 #18742

Closed

hangshao0 reviewed Jan 11, 2024

View reviewed changes

test/functional/Valhalla/playlist.xml Outdated Show resolved Hide resolved

Temporarily disable failing ValhallaAttributeTests

4739d64

Failing due to missing NPE check for pufield in the nullrestricted flattened case. Signed-off-by: Theresa Mammarella <[email protected]>

theresa-m force-pushed the split_ddrlw5 branch from 16c9e97 to 4739d64 Compare January 11, 2024 18:53

hangshao0 approved these changes Jan 12, 2024

View reviewed changes

hangshao0 merged commit 0cbeed0 into eclipse-openj9:master Jan 12, 2024
9 checks passed

theresa-m mentioned this pull request Feb 5, 2024

Set default values for implicit value classes #18892

Merged

theresa-m deleted the split_ddrlw5 branch October 11, 2024 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

valuetypeddrtests with Valhalla lw5 #18597

valuetypeddrtests with Valhalla lw5 #18597

theresa-m commented Dec 11, 2023 •

edited

Loading

hangshao0 commented Dec 12, 2023

theresa-m commented Dec 12, 2023

theresa-m commented Dec 14, 2023

hangshao0 commented Jan 4, 2024

theresa-m commented Jan 4, 2024

hangshao0 commented Jan 4, 2024

hangshao0 commented Jan 4, 2024

theresa-m commented Jan 5, 2024

theresa-m commented Jan 5, 2024

hangshao0 commented Jan 5, 2024

hzongaro commented Jan 8, 2024

theresa-m commented Jan 8, 2024

theresa-m commented Jan 9, 2024

hangshao0 commented Jan 9, 2024

hangshao0 commented Jan 9, 2024

hangshao0 commented Jan 9, 2024

JasonFengJ9 commented Jan 9, 2024

hangshao0 commented Jan 9, 2024

hangshao0 commented Jan 9, 2024

hangshao0 commented Jan 10, 2024 •

edited

Loading

llxia commented Jan 10, 2024

hangshao0 commented Jan 11, 2024

theresa-m commented Jan 11, 2024

hangshao0 commented Jan 11, 2024

theresa-m commented Jan 11, 2024

hangshao0 commented Jan 11, 2024

hangshao0 commented Jan 11, 2024

valuetypeddrtests with Valhalla lw5 #18597

valuetypeddrtests with Valhalla lw5 #18597

Conversation

theresa-m commented Dec 11, 2023 • edited Loading

hangshao0 commented Dec 12, 2023

theresa-m commented Dec 12, 2023

theresa-m commented Dec 14, 2023

hangshao0 commented Jan 4, 2024

theresa-m commented Jan 4, 2024

hangshao0 commented Jan 4, 2024

hangshao0 commented Jan 4, 2024

theresa-m commented Jan 5, 2024

theresa-m commented Jan 5, 2024

hangshao0 commented Jan 5, 2024

hzongaro commented Jan 8, 2024

theresa-m commented Jan 8, 2024

theresa-m commented Jan 9, 2024

hangshao0 commented Jan 9, 2024

hangshao0 commented Jan 9, 2024

hangshao0 commented Jan 9, 2024

JasonFengJ9 commented Jan 9, 2024

hangshao0 commented Jan 9, 2024

hangshao0 commented Jan 9, 2024

hangshao0 commented Jan 10, 2024 • edited Loading

llxia commented Jan 10, 2024

hangshao0 commented Jan 11, 2024

theresa-m commented Jan 11, 2024

hangshao0 commented Jan 11, 2024

theresa-m commented Jan 11, 2024

hangshao0 commented Jan 11, 2024

hangshao0 commented Jan 11, 2024

theresa-m commented Dec 11, 2023 •

edited

Loading

hangshao0 commented Jan 10, 2024 •

edited

Loading