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

[Christian Hagedorn]

On Thu, 4 Apr 2024 15:11:15 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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).
>
>> > 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?

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

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