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

[Roland Westrelin]

On Wed, 10 Apr 2024 14:42:19 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>>> > Should we go back to the previously suggested version without CastPP?
>>> 
>>> Isn't there a risk with that one too? if `superklass` is a `TypeNode` and its input changes to a constant for instance.
>> 
>> I'm afraid you're right. This could probably happen, too.
>> 
>>> If doing this in `GraphKit` doesn't work well, maybe it should be done in `SubTypeCheckNode::Value`? This way it would apply all all stages of compilation? (That assumes we don't care of what happens if `ExpandSubTypeCheckAtParseTime` is true).
>> 
>> I've first thought of doing it in `SubTypeCheckNode::Value()` but assumed we can get away with handling it in `GraphKit`. But as now figured out, this comes with new problems and does not seem to be safe. I will try to undo my current fix idea in `GraphKit` and do it in `SubTypeCheckNode::Value()` instead. This should work (not yet sure though what to do with `ExpandSubTypeCheckAtParseTime` and if it's easy to fix - otherwise, we could move forward with the proposal to remove it for good).
>
> I could not find a test that shows a benefit by doing the improved check with code from `try_improve()` inside `SubTypeCheckNode::Value()`. The original patch only showed a win for mainline when we have two identical `SubTypeCheckNodes` such that they can common up with an improved constant. But this cannot be achieved when having the code in `SubTypeCheckNode::Value()`.
> 
> I therefore suggest to revert back to the original version to directly plug in a better constant, if we find one with `try_improve()`, and just skip the other non-constant cases with `LoadKlass` etc.
> 
> For the Valhalla bug, I can do the more sophisticated fix to improve `SubTypeCheckNode::Value()` with code from `try_improve()` but it does not seem worth for mainline. What do you think?

Sounds good. Thanks for giving it a try.
Since you mention fixing this in valhalla, do you expect it makes a difference there but not in mainline?

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

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