Fix tests in disabled variations of ValueTypeSystemArraycopyTests

[theresa-m]

Once #19911 is merged ValueTypeSystemArraycopyTests will be re-enabled with the first 3 variations, all including Xint. The remaining variations have failures.

FYI @a7ehuo @hzongaro

[a7ehuo]

Just an update on this issue. The investigation is ongoing.

Found one of root causes of the failures is that null restricted arrays that are created through jdk/internal/value/ValueClass.newArrayInstance are not recognized properly in VP and got transformed as regular array in System.arraycopy transformation.

I have a potential fix which fixes 14 out of 19 ValueTypeSystemArraycopyTests failures. The other 5 failures seem related to array flattening.

[theresa-m]

I have a draft going for array flattening #19995

I'm planning to retest and submit this after #19911 is merged.

[a7ehuo]

@theresa-m @hangshao0 @hzongaro with change #19911, array component J9Class has one _arrayClass pointer and one _nullRestrictedArrayClass. Both _arrayClass and _nullRestrictedArrayClass share the same signature, for example, [LSomeValueClass;

In the JIT, it uses getClassFromSignature to get the class based on the signature. getClassFromSignature eventually invokes internalFindClassInModule in classsupport.c. Currently internalFindClassInModule returns the regular array class (the non null restricted array class) for signature [LSomeValueClass;. Would/should there be any change on internalFindClassInModule? If no change, once we know the signature [LSomeValueClass;, we will always only be able to get the regular array class (the non null restricted array class) from internalFindClassInModule. The reason I'm asking this question is that in the JIT, currently once we find the class from the signature, it is concluded a fixed class and a few of VP optimizations are based on it.

[hangshao0]

Would/should there be any change on internalFindClassInModule? If no change, once we know the signature [LSomeValueClass;, we will always only be able to get the regular array class (the non null restricted array class) from internalFindClassInModule.

We could add a new option flag that can be passed to the last parameter of internalFindClassInModule() return the null restricted array.

[a7ehuo]

We could add a new option flag that can be passed to the last parameter of internalFindClassInModule() return the null restricted array.

That would help solve one problem. I think the problem for the JIT here is that when we have a class signature, we have no idea if it is for a regular nullable array class or a null restricted array class. There are assumptions in the JIT that trust the class pointer returned from getClassFromSignature to be fixed to one specific array class, which is no longer true with null restricted arrays where two array classes could be associated with the same signature

[hangshao0]

I think the problem for the JIT here is that when we have a class signature, we have no idea if it is for a regular nullable array class or a null restricted array class. There are assumptions in the JIT that trust the class pointer returned from getClassFromSignature to be fixed to one specific array class, which is no longer true with null restricted arrays where two array classes could be associated with the same signature

@TobiAjila Any thoughts on that ?

[theresa-m]

Not sure if it helps but at one point javac (lw5 branch) was generating Class.asNullRestrictedType before each use of a null restricted array to distinguish the two types of arrays.

[tajila]

I think the problem for the JIT here is that when we have a class signature, we have no idea if it is for a regular nullable array class or a null restricted array class. There are assumptions in the JIT that trust the class pointer returned from getClassFromSignature to be fixed to one specific array class, which is no longer true with null restricted arrays where two array classes could be associated with the same signature

The Null-restricted array doubles down on erasure in that from the perspective of the classfile Foo[] and Foo![] are both [LFoo. This means anywhere [LFoo is encountered you need to be prepared to deal with both nullable and null-restricted unless you can prove it goes one way or the other. In the interpreter we rely on runtime checks for this.

The JIT will need to do something similar. Either prove that it must be Foo![] or Foo[] or be prepared for both. You can prove that its not Foo[] if you know the array slot was populated with Class.asNullRestrictedType or something similar. There are other techniques as well.

In either case looking for the array with [LFoo is the right approach because you can get both array classes from each other.

We could add a new option flag that can be passed to the last parameter of internalFindClassInModule() return the null restricted array.

So Im not really in favour of this, because you need to know what you are looking for, and if you know you can always find it with the regular array class.

[hangshao0]

and if you know you can always find it with the regular array class.

The current code of internalFindClassInModule() only looks for the nullable array. It is possible that only null restricted array J9Class has been created, nullable array J9Class is NULL.

[tajila]

The current code of internalFindClassInModule() only looks for the nullable array. It is possible that only null restricted array J9Class has been created, nullable array J9Class is NULL.

With normal usage partterns, given that javac will lookup the NR class from the nullable one then we should be guaranteed that the nullable one is loaded. But I guess someone can handwrite bytecodes that break this assumption.

What we could do is always load the nullable array class as a pre-requisite for the NR array that way if the JIT looks up [LFoo and and either Foo![] or Foo[] is created there is always an answer.

[a7ehuo]

I found the root cause of why some of the ValueTypeSystemArraycopyTests still fail. The failed subtest is testSystemArrayCopy29. The failures happen when testVTVT is inlined into testSystemArrayCopy29, which inlines System.arraycopy into testSystemArrayCopy29.

In Global Value Propagation, System.arraycopy is transformed into arraycopy with a test n116n to make sure the destination array is not a null-restricted array. The destination array (n6n #401 -> n58n #419 -> n83n #422 -> n108n) here is a null restricted array.

n58n      astore  <temp slot 4>[#419  Auto] [flags 0x20000007 0x0 ] (privatizedInlinerArg )   [     0x3ff9921e1e0] bci=[-1,6,785] rc=0 vc=41 vn=4 li=3 udi=2 nc=1 flg=0x2000
n6n         aload  org/openj9/test/lworld/ValueTypeSystemArraycopyTests.nullRestrictedVtArrayDst [Lorg/openj9/test/lworld/ValueTypeSystemArraycopyTests$SomeValueClass;[#401  Static] [flags 0x307 0x0 ]  [     0x3ff991828e0] bci=[-1,6,785] rc=1 vc=41 vn=4 li=- udi=- nc=0
...
n83n      astore  <temp slot 2>[#422  Auto] [flags 0x7 0x0 ]                                  [     0x3ff9921e9b0] bci=[0,7,218] rc=0 vc=0 vn=59 li=- udi=- nc=1
n51n        aload  <temp slot 4>[#419  Auto] [flags 0x20000007 0x0 ]                          [     0x3ff991836f0] bci=[0,2,218] rc=2 vc=41 vn=4 li=7 udi=6 nc=0
...
...
n116n     ificmpne --> block_10 BBStart at n76n ()                                            [     0x3ff9921f400] bci=[-1,0,784] rc=0 vc=0 vn=92 li=- udi=- nc=2 flg=0x20
n114n       iand                                                                              [     0x3ff9921f360] bci=[-1,0,784] rc=1 vc=0 vn=90 li=- udi=- nc=2
n112n         iloadi  <isClassFlags>[#322  Shadow +36] [flags 0x603 0x0 ]                     [     0x3ff9921f2c0] bci=[-1,0,784] rc=1 vc=0 vn=88 li=- udi=- nc=1
n111n           aloadi  <vft-symbol>[#323  Shadow] [flags 0x18607 0x0 ]                       [     0x3ff9921f270] bci=[-1,0,784] rc=1 vc=0 vn=87 li=- udi=- nc=1
n108n             aload  <temp slot 2>[#422  Auto] [flags 0x7 0x0 ]                           [     0x3ff9921f180] bci=[-1,0,784] rc=1 vc=0 vn=84 li=- udi=- nc=0
n113n         iconst 0x2000000 (X!=0 X>=0 )                                                   [     0x3ff9921f310] bci=[-1,0,784] rc=1 vc=0 vn=89 li=- udi=- nc=0 flg=0x104
n115n       iconst 0 (X==0 X>=0 X<=0 )                                                        [     0x3ff9921f3b0] bci=[-1,0,784] rc=1 vc=0 vn=91 li=- udi=- nc=0 flg=0x302
...

The constraint for the destination array n6n is incorrectly set as the nullable array class (0x1A18A00) instead of the null-restricted array class (0x1A1F700). It caused the test n116n on whether or not the array is a null-restricted array is incorrectly folded away since the iand n114n is zero for nullable array class.

   n6n aload gets new constraint: value 6 is fixed class 0000000001A18A00 [Lorg/openj9/test/lworld/ValueTypeSystemArraycopyTests$SomeValueClass; (regular class 0000000001A18A00 nullRestricted class 0000000001A1F700) (hint 0x0000000001A18A00 [Lorg/openj9/test/lworld/ValueTypeSystemArraycopyTests$SomeValueClass;) (min bound 0, max bound 394313728) (array element size 4) {HeapObject,StackObject}

[    57] O^O VALUE PROPAGATION: Constant folding iand [000003FF9921F360]          to iconst 0
   cmp children have same value number: 000003FF9921F360 = 000003FF9921E460 = 66
[    58] O^O VALUE PROPAGATION: Removing conditional branch [000003FF9921F400] ificmpne

The constrainAload sets up constraint for n6n aload based on classBlock retrieved from class signature. Both nullable array class and null-restricted array class share the same signature and currently the nullable array class is returned. I have a fix in VP to address this bug.

[github-actions]

Issue Number: 19914
Status: Closed
Actual Components: comp:jit, project:valhalla
Actual Assignees: No one :(
PR Assignees: theresa-m, a7ehuo

[github-actions]

Issue Number: 19914
Status: Closed
Actual Components: comp:jit, project:valhalla
Actual Assignees: No one :(
PR Assignees: theresa-m, a7ehuo