The crash happens in the call to `flat_log_elem_size` in 
`TypeAryPtr::add_field_offset_and_offset`

https://github.com/openjdk/valhalla/blob/95bef16235d88773081e16e2093c5c1adca82757/src/hotspot/share/opto/type.cpp#L5796

in `flat_log_elem_size`

https://github.com/openjdk/valhalla/blob/95bef16235d88773081e16e2093c5c1adca82757/src/hotspot/share/opto/type.cpp#L5227-L5229

`as_flat_array_klass()` fails with a "bad cast" assert because `exact_klass()` 
returns something that is not a flat array. This method uses 
`ciObjArrayKlass::make` to find the right kind of array:

https://github.com/openjdk/valhalla/blob/95bef16235d88773081e16e2093c5c1adca82757/src/hotspot/share/opto/type.cpp#L6822-L6841

which then goes to `ciObjArrayKlass::make_impl`, 
`ObjArrayKlass::klass_with_properties` and eventually 
`ObjArrayKlass::array_layout_selection` that is the source of truth for 
selection of array layout (isn't it well-named?!).

The problem is that in `TypeAryPtr::exact_klass_helper`, the described array 
has properties that must be represented by a reference array, and thus 
`flat_log_elem_size` fails to find a flat array. But how is that possible!? 
From `CheckCastPP` nodes.

The first `CheckCastPP` to enter the graph comes from `Parse::array_load` -> 
`InlineTypeNode::make_from_flat_array`. This one will make some control flow to 
split cases whether the array is nullable or not, atomic or not... and for each 
branch, insert a cast to the right kind of array before the actual load. At 
this step, if a combination is impossible, we should already be able to detect 
it.

The second `CheckCastPP` comes from the intrinsicalization of a flavor of 
`newArray` in `LibraryCallKit::inline_newArray`. Eventually, this function will 
create an `AllocateArrayNode` with a `CheckCastPPNode` just under. In my case, 
because of late inlining, when this node is added, the `CheckCastPP` nodes from 
the `array_load` are already there, and all the `CheckCastPPNode` describes 
array shapes that `array_layout_selection` would allow. At this point, the 
first`CheckCastPP` is an output of the second, because late inlining managed to 
rewire the access directly to the allocation.

The tragedy happens during IGVN: the lower `CheckCastPP` (the first added) gets 
a refined type that is the intersection of its own type, and the type of its 
input. One type is "null-free /\ atomic-unknown /\ flat" and the other one is 
"null-unknown /\ atomic /\ flat". The intersection is "null-free /\ atomic /\ 
flat" that is impossible in the case of the failing test (because of an 
explicit `-XX:-UseNullFreeAtomicValueFlattening`). So, when 
`TypeAryPtr::exact_klass_helper` invokes (indirectly) 
`ObjArrayKlass::array_layout_selection` to build the right `ciObjArrayKlass`, 
the latter decides that a null-free atomic array should be a reference array 
(not flat), causing the crash.

It is not much than an example of an abstract element being non-bottom 
(Hotspot's top), but having a bottom (resp. top) concretization. One could 
normalize it on construction, make so that `TypeAryPtr::make` returns directly 
the abstract bottom (resp. top), but dual values are a problem... The solution 
can be simply to enhance `TypeAryPtr::empty()`. In the same way that we detect 
the impossibility of having an array that is both flat and non-flat, we can 
also rule out combination of nullability, atomicity and flatness that are not 
possible. It just needs cooperation of `ObjArrayKlass::array_layout_selection`.

Tested with tier1-4,stress,valhalla-stress + tier1-3 with enable-preview.

Thanks,
Marc

---------
- [x] I confirm that I make this contribution in accordance with the [OpenJDK 
Interim AI Policy](https://openjdk.org/legal/ai).

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

Commit messages:
 - cleanup
 - nicer
 - No caching, seems risky
 - Trying more...
 - Try again
 - Laxer
 - Trying this
 - Unproblemlist

Changes: https://git.openjdk.org/valhalla/pull/2534/files
  Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=2534&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8384262
  Stats: 54 lines in 5 files changed: 52 ins; 1 del; 1 mod
  Patch: https://git.openjdk.org/valhalla/pull/2534.diff
  Fetch: git fetch https://git.openjdk.org/valhalla.git pull/2534/head:pull/2534

PR: https://git.openjdk.org/valhalla/pull/2534

Reply via email to