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

[Emanuel Peter]

On Wed, 17 Jan 2024 10:13:22 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   merge fix
>
> src/hotspot/share/opto/callGenerator.cpp line 888:
> 
>> 886:         } else {
>> 887:           if (c->Opcode() == Op_If) {
>> 888:             Node* bol = c->in(1);
> 
> Suggestion:
> 
>             BoolNode* bol = c->in(1)->as_Bool();
> 
> Then you can drop the assert below

Or even:
`Node* cmp = c->in(1)->as_Bool()->in(1);`

> src/hotspot/share/opto/callGenerator.cpp line 902:
> 
>> 900:                 scoped_value_cache = in1->in(0)->as_Call();
>> 901:               } else {
>> 902:                 assert(scoped_value_cache == in1->in(0), "");
> 
> Suggestion:
> 
>                 assert(scoped_value_cache == in1->in(0), "should only find one scoped_value_cache");

Even nicer might be to also have an assert, and then just assign:

assert(scoped_value_cache == nullptr || scoped_value_cache == in1->in(0), "should only find one scoped_value_cache");
scoped_value_cache = in1->in(0)->as_Call();

> src/hotspot/share/opto/callGenerator.cpp line 923:
> 
>> 921:                 assert(in2 = _sv, "");
>> 922:                 in = in1;
>> 923:               }
> 
> Might be less verbose:
> 
> assert(in1 == _sv || in2 == _sv, "one of the comparison values must be the scoped value (oop?)");
> // pick the other:
> Node* in = (in1 == _sv) ? in2 : in1;
> 
> Could we have a more expressive name for `in`?

Also a general comment about what kind of matching we are doing here would be nice.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1455161369
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1455173243
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1455211335