On Sat, 20 Jun 2026 08:35:18 GMT, Christian Hagedorn <[email protected]> wrote:
>> The provided test cases fail when inlining the `Array.copyOf/copyOfRange()` >> intrinsics where the source array is flat and from an abstract value class. >> >> The current code checks whether the source array or the destination array >> klass contain oops by assuming that a flat value class array is always >> concrete and thus an `InlineKlass` (i.e. can call `inline_klass()`). >> However, we could also have abstract value class arrays that are known to be >> flat (see test cases). This leads to a cast assertion failure because >> abstract value classes are represented by an `InstanceKlass` and not an >> `InlineKlass`. >> >> To fix this, I added a simple bailout when detecting an abstract flat value >> class array. This is a conservative correctness fix and should be revisited >> again post-Valhalla-integration. We have >> [JDK-8251971](https://bugs.openjdk.org/browse/JDK-8251971) in place for that >> which should also tackle other issues around the arraycopy intrinsics and >> also address performance problems. >> >> Thanks, >> Christian >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Christian Hagedorn has updated the pull request incrementally with one > additional commit since the last revision: > > improve checks and further refactor exclude_flat to make it easier to read Sorry for taking some more time to respond here. Thanks for your additional questions @merykitty. I've had another closer look at the code and wrote some more tests and discovered more issues: 1. When passing in primitive type mirrors like `int.class` or for `void.class` to `Arrays.copyOf()`, there is no klass field and we return null: https://github.com/openjdk/valhalla/blob/dc8b7c5c63155cd324933499895cef2507dfe55f/src/hotspot/share/opto/memnode.cpp#L2834-L2837 As a result, we crash here because `klass_node` returned from `load_klass_from_mirror()` is `top`: https://github.com/openjdk/valhalla/blob/dc8b7c5c63155cd324933499895cef2507dfe55f/src/hotspot/share/opto/library_call.cpp#L5195 - Solution: Bail out if `stopped()` after `load_klass_from_mirror()`. 2. When passing in an instance class mirror like `A.class` we should throw because it's not an array class. However, with the checks in the current code, we only emit a type array guard which does not bail out when having an instance klass: https://github.com/openjdk/valhalla/blob/dc8b7c5c63155cd324933499895cef2507dfe55f/src/hotspot/share/opto/library_call.cpp#L5196-L5201 We then try to load the refined array klass from an instance klass pointer in `load_default_refined_array_klass()` where we read from `ObjArrayKlass::next_refined_array_klass_offset()` which is garbage. - Solution: Emit `generate_non_refArray_guard()` when the klass pointer is not an array klass. 3. We need to check `UseArrayFlattening` and the GC barrier availability after the inserted check for for 2., otherwise, we hit the same problem again when disabling `UseArrayFlattening` or not requiring GC barriers at all. I pushed an update with the following changes: - Refactored bailout logic again to a separate method where I check all the cases systematically: - Check dest klass to be an array klass. - Check all cases for maybe flat and bail out which also covers `j.l.Object`. - Check write barriers. - Added more comments. - Added a lot more tests by using the Template Framework. Testing this through t1-4 + stress looked good. Since we now bail out in more cases, I might also run some performance testing. Let me know, what you think. ------------- PR Review: https://git.openjdk.org/valhalla/pull/2569#pullrequestreview-4606939751
