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

[Emanuel Peter]

On Thu, 8 Feb 2024 07:53:31 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 866:
> 
>> 864:   // if (objects[n] == this) {
>> 865:   bool cache_probe(Node* c) {
>> 866:     if (c->Opcode() == Op_If) {
> 
> Please choose a better name for c. And bail out instead of nesting.

Also: this is another `parse` method, right?

> src/hotspot/share/opto/callGenerator.cpp line 903:
> 
>> 901: 
>> 902:       Node* index_in_cache_array = _kit->gvn().intcon(const_offset >> shift);
>> 903:       if (addp2->is_AddP()) {
> 
> You either need a comment here, or maybe better variable names can help.
> Currently, I can't really review this / convince myself you are covering all cases without reverse-engineering all variable names and the code.

Obviously, you are trying to find the `int` type index into the cache array here. But what happens in the else-branch here? Why can we just ignore `addp2`?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482548361
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482580821