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

Value Types: Transformation of System.arraycopy doesn't handle flattened value type array correctly #17586

Closed
a7ehuo opened this issue Jun 13, 2023 · 3 comments
Assignees
Labels
comp:jit project:valhalla Used to track Project Valhalla related work

Comments

@a7ehuo
Copy link
Contributor

a7ehuo commented Jun 13, 2023

I have a simple code that tests System.arraycopy with flattened value type arrays [1]. The data in the destination array after System.arraycopy is incorrect with the JIT'd method. If the array is an interface array with flattened value type array component, the result is also incorrect [2]. I suspect there might be some miscalculation on offsets when System.arraycopy is transformed into arraycopy instructions.

-----testVTVT: INT VT => VT----- (-Xint: good)
10.0
9.0
8.0
7.0
6.0
5.0
4.0
3.0
2.0
1.0
-----testVTVT: JIT VT => VT----- (bad)
10.0
9.0
8.0
7.0
6.0
5.0
6.0 // wrong
7.0 // wrong
8.0 // wrong
9.0 // wrong

[1]

primitive public class SomePrimitiveValueClass implements SomeInterface {
   double val;

   SomePrimitiveValueClass(double i) {
      this.val = i;
   }

   public void print() {
      System.out.println(this.val);
   }
}

static void testVTVT(SomePrimitiveValueClass[] dst, SomePrimitiveValueClass[] src) {
     System.arraycopy(src, 0, dst, 0, ARRAY_SIZE);
}

public static void main(String[] args) {
      SomePrimitiveValueClass[] arr1 = new SomePrimitiveValueClass[ARRAY_SIZE];
      SomePrimitiveValueClass[] arr2 = new SomePrimitiveValueClass[ARRAY_SIZE];

      for (int i=0; i < ARRAY_SIZE; i++) {
         arr1[i] = new SomePrimitiveValueClass((double)i);
         arr2[i] = new SomePrimitiveValueClass((double)(ARRAY_SIZE-i));
      }

    testVTVT(arr1, arr2);
}

[2]

-----testVTIFIF: JIT IF => IF (VT) 1-----
-1.6188120589001373E308 
-1.6190863651036408E308
-1.6193606713071444E308
-1.619634977510648E308
-1.6199092837141515E308
5.0
6.0
7.0
8.0
9.0
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Jun 13, 2023

@hzongaro fyi

@hangshao0 hangshao0 added the project:valhalla Used to track Project Valhalla related work label Jun 13, 2023
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Jun 14, 2023

The root cause of the issue is that the existing implementation of the transformation of System.arraycopy doesn't take into consideration of flattened primitive value types.

When transformArrayCopyCall calculates element size, it looks at array component class type. If it is a reference array, the element size is always 4 (n43n) [1].

The flattened primitive value type array is considered reference array here, but the element size should be the total size of all fields of the array component class. For the above test, there is one field val, which is double, in SomePrimitiveValueClass. Once it's embedded into the array SomePrimitiveValueClass[], the array element size should be 8 instead of 4.

To make the transformation of System.arraycopy function correctly for flattened value types, we have to check during the compilation on whether or not it accesses flattened value type arrays and calculate element size based on the total field size of the array component class. If we are not sure if the array component type might or might not be flattened value types during the compilation time, we cannot directly transform System.arraycopy as it is today. I will add the fix for this issue as part of the ongoing prototype work for #17196.

[1]

n9n       treetop                                                                             [0x7f442d48e850] bci=[-1,7,10] rc=0 vc=170 vn=- li=-2 udi=- nc=1
n8n         arraycopy  java/lang/System.arraycopy(Ljava/lang/Object;ILjava/lang/Object;II)V[#427  final native static Method] [flags 0x20500 0x0 ] (Unsigned forwardArrayCopy noArrayStoreCheckArrayCopy referenceArrayCopy )  [0x7f442d48e800] bci=[-1,7,10] rc=1 vc=170 vn=- li=-1 udi=- nc=5 flg=0xc020
n3n           ==>aload
n5n           ==>aload
n41n          aladd (internalPtr )                                                            [0x7f442d48f250] bci=[-1,0,10] rc=1 vc=0 vn=- li=- udi=- nc=2 flg=0x8000
n3n             ==>aload
n39n            lconst 8 (highWordZero X!=0 X>=0 )                                            [0x7f442d48f1b0] bci=[-1,7,10] rc=2 vc=0 vn=- li=- udi=- nc=0 flg=0x4104
n42n          aladd (internalPtr )                                                            [0x7f442d48f2a0] bci=[-1,2,10] rc=1 vc=0 vn=- li=- udi=- nc=2 flg=0x8000
n5n             ==>aload
n39n            ==>lconst 8
n45n          lmul                                                                            [0x7f442d48f390] bci=[-1,4,10] rc=1 vc=0 vn=- li=- udi=- nc=2
n44n            i2l                                                                           [0x7f442d48f340] bci=[-1,4,10] rc=1 vc=0 vn=- li=- udi=- nc=1
n7n               ==>iload
n43n            lconst 4 (highWordZero X!=0 X>=0 ) 

@a7ehuo a7ehuo self-assigned this Jun 14, 2023
a7ehuo added a commit to a7ehuo/omr that referenced this issue Aug 3, 2023
Required by JEP 401: If null restricted value type is enabled,
we need to preform null check on the value being stored in order to
throw a `NullPointerException` if the array is null-restricted
and the value to write is null. If it is this case,
`System.arraycopy` cannot be transformed into arraycopy instructions.

Based on whether or not the source array and the destination array
are null restricted value type, and whether or not they are flattened,
VP decides whether or not transform `System.arraycopy`.

Fixes: eclipse-openj9/openj9#17196,eclipse-openj9/openj9#17586

Signed-off-by: Annabelle Huo <[email protected]>
a7ehuo added a commit to a7ehuo/omr that referenced this issue Aug 16, 2023
Required by JEP 401: If null restricted value type is enabled,
we need to preform null check on the value being stored in order to
throw a `NullPointerException` if the array is null-restricted
and the value to write is null. If it is this case,
`System.arraycopy` cannot be transformed into arraycopy instructions.

Based on whether or not the source array and the destination array
are null restricted value type, and whether or not they are flattened,
VP decides whether or not transform `System.arraycopy`.

Fixes: eclipse-openj9/openj9#17196,eclipse-openj9/openj9#17586

Signed-off-by: Annabelle Huo <[email protected]>
a7ehuo added a commit to a7ehuo/omr that referenced this issue Aug 17, 2023
Required by JEP 401: If null restricted value type is enabled,
we need to preform null check on the value being stored in order to
throw a `NullPointerException` if the array is null-restricted
and the value to write is null. If it is this case,
`System.arraycopy` cannot be transformed into arraycopy instructions.

Based on whether or not the source array and the destination array
are null restricted value type, and whether or not they are flattened,
VP decides whether or not transform `System.arraycopy`.

Fixes: eclipse-openj9/openj9#17196,eclipse-openj9/openj9#17586

Signed-off-by: Annabelle Huo <[email protected]>
a7ehuo added a commit to a7ehuo/omr that referenced this issue Aug 22, 2023
Required by JEP 401: If null restricted value type is enabled,
we need to preform null check on the value being stored in order to
throw a `NullPointerException` if the array is null-restricted
and the value to write is null. If it is this case,
`System.arraycopy` cannot be transformed into arraycopy instructions.

Based on whether or not the source array and the destination array
are null restricted value type, and whether or not they are flattened,
VP decides whether or not transform `System.arraycopy`.

Fixes: eclipse-openj9/openj9#17196,eclipse-openj9/openj9#17586

Signed-off-by: Annabelle Huo <[email protected]>
a7ehuo added a commit to a7ehuo/omr that referenced this issue Aug 22, 2023
Required by JEP 401: If null restricted value type is enabled,
we need to preform null check on the value being stored in order to
throw a `NullPointerException` if the array is null-restricted
and the value to write is null. If it is this case,
`System.arraycopy` cannot be transformed into arraycopy instructions.

Based on whether or not the source array and the destination array
are null restricted value type, and whether or not they are flattened,
VP decides whether or not transform `System.arraycopy`.

Fixes: eclipse-openj9/openj9#17196,eclipse-openj9/openj9#17586

Signed-off-by: Annabelle Huo <[email protected]>
@hzongaro
Copy link
Member

Fixed by OMR pull request eclipse-omr/omr#7084

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

No branches or pull requests

3 participants