RFR: 8328480: C2: SubTypeCheckNode in checkcast should use the klass constant of a unique concrete sub class

[Christian Hagedorn]

On Wed, 27 Mar 2024 16:15:08 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> While working on a [Valhalla bug](https://bugs.openjdk.org/browse/JDK-8321734), I've noticed that a `SubTypeCheckNode` for a `checkcast` does not take a unique concrete sub class `X` of an abstract class `A` as klass constant in the sub type check. Instead, it uses the abstract klass constant:
>> 
>> 
>> abstract class A {}
>> class X extends A {}
>> 
>> A x = (A)object; // Emits SubTypeCheckNode(object, A), but could have used X instead of A.
>> 
>> However, the `CheckCastPP` result already uses the improved instance type ptr `X` (i.e. `toop` which was improved from `A` by calling `try_improve()` to get the unique concrete sub class):
>> https://github.com/openjdk/jdk/blob/614db2ea9e10346475eef34629eab54878aa482d/src/hotspot/share/opto/graphKit.cpp#L3257-L3261
>> https://github.com/openjdk/jdk/blob/614db2ea9e10346475eef34629eab54878aa482d/src/hotspot/share/opto/graphKit.cpp#L3363
>> 
>> We should also plug in a unique concrete sub class constant in the `SubTypeCheckNode` which could be beneficial to fold away redundant sub type checks (see test cases).
>> 
>> This fix is required to completely fix the bug in Valhalla (this is only one of the broken cases). In Valhalla, the graph ends up being broken because a `CheckCastPP` node is folded because of an impossible type but the `SubTypeCheckNode` is not due to not using the improved unique concrete sub class constant for the `checkcast`. I don't think that there is currently a bug in mainline because of this limitation - it just blocks some optimizations. I'm therefore upstreaming this fix to mainline since it can be beneficial to have this fix here as well (see test cases).
>> 
>> Thanks,
>> Christian
>
> src/hotspot/share/opto/graphKit.cpp line 3362:
> 
>> 3360:     // Generate the subtype check
>> 3361:     Node* improved_superklass = superklass;
>> 3362:     if (improved_klass_ptr_type != klass_ptr_type && improved_klass_ptr_type->singleton()) {
> 
> In what case can there be an `improved_klass_ptr_type` that's not a constant?

I was surprised by that as well when I've run testing but `gen_checkcast()` is actually also called in `Parse::array_store_check()` which passes a `LoadKlassNode` which is not a constant:

https://github.com/openjdk/jdk/blob/05854fd704cba6ebd73007d9547a064891d49587/src/hotspot/share/opto/parseHelper.cpp#L233-L238

It states that it ignores the result and just calls it for the CFG effects - does not sound like a very clean solution.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18515#discussion_r1541468186