RFR: 8308869: C2: use profile data in subtype checks when profile has more than one class [v6]

[Roland Westrelin]

On Fri, 23 Jun 2023 18:47:59 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision:
>> 
>>  - more test failures
>>  - Merge branch 'master' into JDK-8308869
>>  - whitespaces
>>  - test failures
>>  - review
>>  - 32 bit fix
>>  - white spaces
>>  - fix & test
>
> Overall, I'd prefer to leave commoning considerations for a separate enhancement. 
> 
> Embedding `ciCallProfile` looks to me much cleaner than exploding its content into node inputs.
> 
> Having profile info explicitly fed into `SubTypeCheck` as node inputs in practice defeats any possible sharing unless the nodes are constructed from the very same profile data. The types, their order, and frequencies have to perfectly match in order for commoning to happen. You already have  `IfNode::same_condition()` to alleviate some of the effects of broken sharing.
> 
> When you embed profiling info you are left with a choice how to common nodes (whether to take profiling info into account or not). But if you simply ignore it until macro expansion, the behavior will stay the same as it is now.
> 
> I prefer the patch to be focused on slow path case (reduce the frequency of secondary super cache checks & updates) and leave the rest for future considerations.  
> 
> As an example, it's still an open question for me should `IfNode::search_identical()` take profile info into account. Current patch ignores profile-related info (`IfNode::same_condition()` check), but maybe it is worth merging the profiles instead?  
> 
> 
>> Some SubTypeCheck nodes have no profile data associated with them.
> 
> I don't consider footprint as an issue here. `SubTypeCheck`s are relatively rare and `ciCallProfile` size is quite small for any practical morphism limits. Additional profiling may introduce more about 1-2 additional slots (rather than 10s or 100s) and the main footprint hit will be on runtime side (in MDOs). 
> 
>> Is that out of concern that getting the code done on all platforms will be too complicated?
> 
> It does look like an excessive requirement, but I'm not too much concerned about it. If you think it's better to get the full support all at once, I'm perfectly fine with that. It just seems cleaner to refine profiling part separately. There are open questions which may be well out of scope for the proposed enhancement. 
> 
> For example, while `checkcast`/`aastore` behave very similarly to `invokevirtual`/`invokeinterface` (very low rate of failures), `instanceof` is different and can expose very high rates of failures (esp. in case of chained `instanceof` checks). Should we continue profiling for that? (Can C2 benefit from such info? I believe so: we could skip SSC check if failure rate is too high.) 
> 
> Also, I refrained from commenting on naming, but `ciCallProfile` does look confusing when it comes to `checkcas...

@iwanowww thanks for looking at this some more.

> I don't see much value in special handling for nodes without associated bytecode location info.

Right. I removed it.

> What's the plan if we agree on adjusting profile collection? Should all the platforms be updated all at once? If not, how is it intended to work during transition period?

I thought it would be better to update profile collection on all platforms as part of this PR initially so no platforms are left behind. But that's likely unrealistic. I pushed a new commit that I think would support both ways of doing profiling so there's no need to have all platforms in sync. What do you think?

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

PR Comment: https://git.openjdk.org/jdk/pull/14375#issuecomment-1630912653