[lworld+vector] RFR: 8311610: [lworld+vector] Clean-up of vector allocation in class VectorSupport [v2]

[Jatin Bhateja]

On Fri, 7 Jul 2023 13:14:33 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

>> PR [1] has added the vector supports for field re-assigning after object re-allocation in `deoptimization.cpp`. The similar vector
>> and vector payload re-allocation code in `vectorSupport.cpp` can be removed now.
>> 
>> Besides, there is an issue in current vector payload allocation code in `vectorSupport.cpp`, that the multi-fields are forgotten
>> to be assigned after the payload object is allocated (see [2]). This method is called separately during deoptimization (see [3]).
>> This makes several Vector API jtreg test results incorrect. And either assigning the fields by calling `Deoptimization::reassign_fields_by_klass()` after it or directly going to the normal re-allocation path in `deoptimization.cpp` can fix the issue. I prefer the latter one. Hence the similar code in `vectorSupport.cpp` is needless and can be removed.
>> 
>> This patch also fixes a crash when printing out the JVM state, which is caused by the additional multi-fields passed to a safepoint. Normally, the fields number passed to a safepoint is the same with the klass's non-static fields number. But it's different for multi-fields. Only the `multifield_base` is added to the klass's non-static field list. Hence if all the un-vectorized multi-fields are passed to a safepoint, the fields number is larger than the klass's non-static fields number. And the array access at [4] throws exception due to out of range index.
>> 
>> [1] https://github.com/openjdk/valhalla/pull/866
>> [2] https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/prims/vectorSupport.cpp#L281
>> [3] https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/runtime/deoptimization.cpp#L1282
>> [4] https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/opto/callnode.cpp#L527
>
> Xiaohong Gong has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Set larval state for inline type objects

> > > Besides the vector api regressions, I met lots of crashes due to nullptr of the payload field. It looks like being caused by deoptmization as well, where the payload field is not allocated I guess. With the change in this PR, the crash cases increased. Do you have any idea about this?
> > 
> > 
> > Can you try integrating #884 locally and check if it resolves the crashes.
> 
> Thanks for the patch! I tested with it, but unfortunatelly, the crash is still there. I will look at this issue today. Thanks!

> > > Besides the vector api regressions, I met lots of crashes due to nullptr of the payload field. It looks like being caused by deoptmization as well, where the payload field is not allocated I guess. With the change in this PR, the crash cases increased. Do you have any idea about this?
> > 
> > 
> > Can you try integrating #884 locally and check if it resolves the crashes.
> 
> Thanks for the patch! I tested with it, but unfortunatelly, the crash is still there. I will look at this issue today. Thanks!

Are you referring to following assertion failures which are seen without -Xshare:off

test Float256VectorTests.unsliceUnaryFloat256VectorTests(float[-i * 5]): failure
java.lang.NoClassDefFoundError: Could not initialize class jdk.internal.vm.vector.VectorSupport$VectorPayloadMF256F
	at java.base/jdk.internal.vm.vector.VectorSupport$VectorPayloadMF.newInstanceFactory(VectorSupport.java:220)
	at jdk.incubator.vector/jdk.incubator.vector.AbstractSpecies.makeDummyVectorMF(AbstractSpecies.java:369)
	at jdk.incubator.vector/jdk.incubator.vector.AbstractSpecies.dummyVectorMF(AbstractSpecies.java:327)
	at jdk.incubator.vector/jdk.incubator.vector.FloatVector$FloatSpecies.dummyVectorMF(FloatVector.java:3859)
	at jdk.incubator.vector/jdk.incubator.vector.FloatVector.fromArray(FloatVector.java:2894)
	at Float256VectorTests.unsliceUnaryFloat256VectorTests(Float256VectorTests.java:3633)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
	at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599)
	at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174)
	at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)

-------------

PR Comment: https://git.openjdk.org/valhalla/pull/882#issuecomment-1628019413