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

[Emanuel Peter]

On Thu, 8 Feb 2024 11:49:51 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 ten commits:
>> 
>>  - review comment
>>  - Merge branch 'master' into JDK-8320649
>>  - Update src/hotspot/share/opto/callGenerator.cpp
>>    
>>    Co-authored-by: Emanuel Peter <emanuel.peter at oracle.com>
>>  - Update test/hotspot/jtreg/compiler/c2/irTests/TestScopedValue.java
>>    
>>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>>  - merge fix
>>  - Merge branch 'master' into JDK-8320649
>>  - test failures
>>  - white spaces + bug id in test
>>  - test & fix
>
> src/hotspot/share/opto/callGenerator.cpp line 935:
> 
>> 933:   // an uncommon trap, it could reach either the first or the second cache probe if first. Figure out which is the first
>> 934:   // here.
>> 935:   void find_first_probe_if(Unique_Node_List &wq) {
> 
> Suggestion:
> 
>   void find_first_probe_if(const Unique_Node_List &wq) {
> 
> I don't think you are modifying `wq`?
> What are the assumptions about `wq`? A better name would help here.

This method does more than "find". Adjust the name to explain the side-effect.
Because technically, we already found it, and actually need the reference to the first probe if.
But we adjust the order of the first and second probe if.
`adjust_order_or_first_and_second_probe_if`?

> src/hotspot/share/opto/callGenerator.cpp line 990:
> 
>> 988:   // Thread.scopedValueCache() which acts as a marker for the beginning of the subgraph of interest. In the process a
>> 989:   // number of checks from the java code of ScopedValue.get() are expected to be encountered. They are recorded:
>> 990:   void pattern_match() {
> 
> Either rename to `parse_...`, or replace my `parse` suggestion with `pattern_match`

Suggestion:

  void parse_scoped_value_get() {

> src/hotspot/share/opto/callGenerator.cpp line 1005:
> 
>> 1003:         }
>> 1004:       } else {
>> 1005:         if (cache_null_check(c)) {
> 
> Suggestion:
> 
>         if (parse_cache_null_check(c)) {

Why not `else if` block? Would lead to less nesting. Other cases below should also be `else if` blocks I think.

> src/hotspot/share/opto/callGenerator.cpp line 1009:
> 
>> 1007:           continue;
>> 1008:         }
>> 1009:         if (!cache_probe(c)) {
> 
> Suggestion:
> 
>         if (!parse_cache_probe(c)) {

What happens in the "else" case of this block? Can that even happen?
Could there be any other `IfNode` here? Add asserts if not.

> src/hotspot/share/opto/callGenerator.cpp line 1024:
> 
>> 1022:           }
>> 1023:         }
>> 1024:         wq.push(c->in(0));
> 
> Given the complicated if/else/continue blocks, I'm not sure when the recursion happens.
> Can you refactor this somehow? Maybe if you do what I suggested above that would already suffice.

Nesting is really too deep here to comfortably read the code

> src/hotspot/share/opto/callGenerator.cpp line 1172:
> 
>> 1170:       phi_cache_value->init_req(2, slow_projs.resproj);
>> 1171:     }
>> 1172:     region_fast_slow->init_req(1, in_cache);
> 
> Move this line down to the 3 phis, so we have it all adjacent, like above.

maybe all of the `init_req(1` should happen before `init_req(2`?

> src/hotspot/share/opto/callGenerator.cpp line 1224:
> 
>> 1222:   }
>> 1223: 
>> 1224:   void remove_thread_scoped_cache_call(const CallProjections& scoped_value_cache_projs) const {
> 
> What is the "thread" for?

And really you are only removing it on some paths: io and merged_memory. What about everything else?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482847646
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482881107
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482889214
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482905173
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482893415
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1483024419
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482975494