RFR: 8320649: C2: Optimize scoped values [v15]

[Emanuel Peter]

On Thu, 18 Apr 2024 11:46:44 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 22 commits:
>> 
>>  - Merge branch 'master' into JDK-8320649
>>  - review
>>  - test fix
>>  - test fix
>>  - Merge branch 'master' into JDK-8320649
>>  - whitespaces
>>  - review
>>  - Merge branch 'master' into JDK-8320649
>>  - review
>>  - 32 bit build fix
>>  - ... and 12 more: https://git.openjdk.org/jdk/compare/bfff02ee...a4ffc11e
>
> src/hotspot/share/opto/loopnode.hpp line 1801:
> 
>> 1799:                                     Node*&second_index,
>> 1800:                                     float &prob_cache_miss_at_first_if, float &first_if_cnt,
>> 1801:                                     float &prob_cache_miss_at_second_if, float &second_if_cnt) const;
> 
> Suggestion:
> 
>   void find_most_likely_cache_index(const ScopedValueGetHitsInCacheNode* hits_in_cache, Node*& first_index,
>                                     Node*& second_index,
>                                     float& prob_cache_miss_at_first_if, float& first_if_cnt,
>                                     float& prob_cache_miss_at_second_if, float& second_if_cnt) const;

That is also what you have at the definition.

> src/hotspot/share/opto/loopopts.cpp line 3783:
> 
>> 3781:             // ScopedValueGetLoadFromCache and companion ScopedValueGetHitsInCacheNode must stay together
>> 3782:             move_scoped_value_nodes_to_not_peel(peel, not_peel, peel_list, sink_list, i);
>> 3783:             incr = false;
> 
> Do we not have to increment the `cloned_for_outside_use`, which affects the `estimate`?

Could we otherwise exhaust the node limit, by peeling a loop that is too large?

> src/hotspot/share/opto/loopopts.cpp line 3997:
> 
>> 3995: }
>> 3996: 
>> 3997: void PhaseIdealLoop::move_scoped_value_nodes_to_not_peel(VectorSet &peel, VectorSet &not_peel, Node_List &peel_list,
> 
> Can you please add more comments to help the reader understand? So we are not peeling in this case?

Maybe rename to `move_scoped_value_nodes_to_avoid_peeling_it`

> src/hotspot/share/opto/loopopts.cpp line 4010:
> 
>> 4008:   peel.remove(hits_in_cache->_idx);
>> 4009:   not_peel.set(hits_in_cache->_idx);
>> 4010:   peel_list.remove(i);
> 
> Looks like duplicated code from the call-site. A refactoring may help.

I think you could combine the code with the case:
`if (n->in(0) == nullptr && !n->is_Load() && !n->is_CMove()) {`
And then you would have this code here, as well as the `TracePartialPeeling` code shared for both.

> src/hotspot/share/opto/subnode.hpp line 341:
> 
>> 339:     assert(req() == Index1, "wrong of inputs for ScopedValueGetHitsInCacheNode");
>> 340:     add_req(index1);
>> 341:     assert(req() == Index2, "wrong of inputs for ScopedValueGetHitsInCacheNode");
> 
> Suggestion:
> 
>     assert(req() == Index2, "wrong number of inputs for ScopedValueGetHitsInCacheNode");

same for the others

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1570567702
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1570620922
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1570552244
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1570562435
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1570441200