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

Thu Feb 8 09:02:00 UTC 2024

On Wed, 7 Feb 2024 08:06:09 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> This change implements C2 optimizations for calls to
>> ScopedValue.get(). Indeed, in:
>> 
>> 
>> v1 = scopedValue.get();
>> ...
>> v2 = scopedValue.get();
>> 
>> 
>> `v2` can be replaced by `v1` and the second call to `get()` can be
>> optimized out. That's true whatever is between the 2 calls unless a
>> new mapping for `scopedValue` is created in between (when that happens
>> no optimizations is performed for the method being compiled). Hoisting
>> a `get()` call out of loop for a loop invariant `scopedValue` should
>> also be legal in most cases.
>> 
>> `ScopedValue.get()` is implemented in java code as a 2 step process. A
>> cache is attached to the current thread object. If the `ScopedValue`
>> object is in the cache then the result from `get()` is read from
>> there. Otherwise a slow call is performed that also inserts the
>> mapping in the cache. The cache itself is lazily allocated. One
>> `ScopedValue` can be hashed to 2 different indexes in the cache. On a
>> cache probe, both indexes are checked. As a consequence, the process
>> of probing the cache is a multi step process (check if the cache is
>> present, check first index, check second index if first index
>> failed). If the cache is populated early on, then when the method that
>> calls `ScopedValue.get()` is compiled, profile reports the slow path
>> as never taken and only the read from the cache is compiled.
>> 
>> To perform the optimizations, I added 3 new node types to C2:
>> 
>> - the pair
>>   ScopedValueGetHitsInCacheNode/ScopedValueGetLoadFromCacheNode for
>>   the cache probe
>>   
>> - a cfg node ScopedValueGetResultNode to help locate the result of the
>>   `get()` call in the IR graph.
>> 
>> In pseudo code, once the nodes are inserted, the code of a `get()` is:
>> 
>> 
>> hits_in_the_cache = ScopedValueGetHitsInCache(scopedValue)
>> if (hits_in_the_cache) {
>>   res = ScopedValueGetLoadFromCache(hits_in_the_cache);
>> } else {
>>   res = ..; //slow call possibly inlined. Subgraph can be arbitray complex
>> }
>> res = ScopedValueGetResult(res)
>> 
>> 
>> In the snippet:
>> 
>> 
>> v1 = scopedValue.get();
>> ...
>> v2 = scopedValue.get();
>> 
>> 
>> Replacing `v2` by `v1` is then done by starting from the
>> `ScopedValueGetResult` node for the second `get()` and looking for a
>> dominating `ScopedValueGetResult` for the same `ScopedValue`
>> object. When one is found, it is used as a replacement. Eliminating
>> the second `get()` call is achieved by making
>> `ScopedValueGetHitsInCache` always successful if there's a dominating
>> `Scoped...
>
> 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/gc/shared/c2/barrierSetC2.hpp line 190:

> 188: class C2OptAccess: public C2Access {
> 189:   PhaseGVN& _gvn;
> 190:   Node* _mem;

Hmm. It is a little strange to have a `_mem` node that is not even a `MemNode*`. What types are expected here?

src/hotspot/share/opto/callGenerator.cpp line 842:

> 840:     }
> 841:     return false;
> 842:   }

Suggestion:

  bool parse_cache_null_check_with_input(IfNode* iff, Node* maybe_cache, Node* maybe_nullptr) {
    if (!maybe_cache->is_Proj() ||
        !maybe_cache->in(0)->is_Call() ||
        maybe_cache->in(0)->as_CallJava()->method()->intrinsic_id() != vmIntrinsics::_scopedValueCache) {
      return false; // parse failed
    }
    assert(maybe_nullptr->bottom_type() == TypePtr::NULL_PTR, "should be a test with null");
    assert(_cache_not_null_iff == nullptr, "should only find one get_cache_if");
    _cache_not_null_iff = iff;
    assert(_scoped_value_cache == nullptr || _scoped_value_cache == maybe_cache->in(0),
           "should only find one scoped_value_cache");
    _scoped_value_cache = maybe_cache->in(0)->as_Call();
    return true; // parse success
  }

`cache_null_check_with_input` sounds like we are going to cache something inside this method. Hmm. Well I guess we sort of are. But really you are parsing the "cache_null_check_with_input".

And please avoid using single-letter variables. Nice names help understanding. We can afford the extra characters ;)

Plus: check preconditions at top, and "bailout" immediately. Nesting is nasty ;)

src/hotspot/share/opto/callGenerator.cpp line 861:

> 859:     }
> 860:     return false;
> 861:   }

Suggestion:

  bool parse_is_cache_null_check(Node* maybe_iff) {
    IfNode* iff = maybe_iff->isa_If();
    if (iff == nullptr) {
      return false; // parse failed
    }
    BoolNode* bol = iff->in(1)->as_Bool();
    Node* cmp = bol->in(1);
    assert(cmp->Opcode() == Op_CmpP, "only reference comparisons in ScopedValue.get()");
    Node* cmp_in1 = cmp->in(1)->uncast();
    Node* cmp_in2 = cmp->in(2)->uncast();
    if (parse_cache_null_check_with_input(iff, cmp_in1, cmp_in2)) {
      return true; // parse success
    } else if (parse_cache_null_check_with_input(iff, cmp_in2, cmp_in1)) {
      return true; // parse success
    }
    return false; // parse failed
  }

Same as above

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.

src/hotspot/share/opto/callGenerator.cpp line 871:

> 869:       assert(cmp->Opcode() == Op_CmpP, "only reference comparisons in ScopedValue.get()");
> 870:       Node* in1 = cmp->in(1)->uncast();
> 871:       Node* in2 = cmp->in(2)->uncast();

Later in the code I will forget what this was the `in` from. Please add a `cmp_` prefix.

src/hotspot/share/opto/callGenerator.cpp line 881:

> 879:         in = in->in(1);
> 880:       }
> 881:       assert(in->Opcode() == Op_LoadP || in->Opcode() == Op_LoadN, "load from cache array expected");

Difficult to keep in my head what `in` means. Maybe it should be called `cache_array_load`?

src/hotspot/share/opto/callGenerator.cpp line 884:

> 882:       assert(_kit->C->get_alias_index(in->adr_type()) == _kit->C->get_alias_index(TypeAryPtr::OOPS),
> 883:              "load from cache array expected");
> 884:       AddPNode* addp1 = in->in(MemNode::Address)->as_AddP();

Suggestion:

      AddPNode* cache_array_load_adr = in->in(MemNode::Address)->as_AddP();

src/hotspot/share/opto/callGenerator.cpp line 885:

> 883:              "load from cache array expected");
> 884:       AddPNode* addp1 = in->in(MemNode::Address)->as_AddP();
> 885:       ProjNode* sv_cache_proj = addp1->in(AddPNode::Base)->uncast()->as_Proj();

Suggestion:

      ProjNode* sv_cache_adr_proj = addp1->in(AddPNode::Base)->uncast()->as_Proj();

This is really the address of the cache, right?

src/hotspot/share/opto/callGenerator.cpp line 894:

> 892:              "load from cache expected right after Thread.scopedValueCache() call");
> 893:       Node* addp2 = addp1->in(AddPNode::Address);
> 894:       Node* offset1 = addp1->in(AddPNode::Offset);

Please give these better names, that tell me what they are right away. Numbering 1 and 2: is that the same as first and second query to cache? I don't think so, right?

src/hotspot/share/opto/callGenerator.cpp line 898:

> 896:       BasicType bt = TypeAryPtr::OOPS->array_element_basic_type();
> 897:       int shift = exact_log2(type2aelembytes(bt));
> 898:       int header = arrayOopDesc::base_offset_in_bytes(bt);

shift and header of what?

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.

src/hotspot/share/opto/callGenerator.cpp line 1005:

> 1003:         }
> 1004:       } else {
> 1005:         if (cache_null_check(c)) {

Suggestion:

        if (parse_cache_null_check(c)) {

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482480355
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482499747
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482501724
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482547292
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482547936
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482550030
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482553463
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482554598
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482571767
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482573609
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482579209
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482545830