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

[Emanuel Peter]

On Wed, 17 Jan 2024 14:27:32 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/classfile/vmIntrinsics.hpp line 301:
> 
>> 299:    do_name(     slowGet_name,                                    "slowGet")                                             \
>> 300:   do_intrinsic(_SVCacheInvalidate,       java_lang_ScopedValue_Cache, invalidate_name, int_void_signature, F_S)         \
>> 301:    do_name(     invalidate_name,                                 "invalidate")                                          \
> 
> What prevents us from writing out `_scopedValueGet` etc, just like the other names here?

Thanks for the update ✔

> src/hotspot/share/gc/shared/c2/barrierSetC2.cpp line 109:
> 
>> 107:     Node* ctl = opt_access.ctl();
>> 108:     assert(opt_access.mem()->is_MergeMem(), "");
>> 109:     MergeMemNode* mm = opt_access.mem()->as_MergeMem();
> 
> Does `as_MergeMem` not assert `is_MergeMem`?

Thanks for the update ✔

> src/hotspot/share/opto/cfgnode.hpp line 737:
> 
>> 735:   ProjNode* result_out() {
>> 736:     return proj_out_or_null(Result);
>> 737:   }
> 
> Either verify that we have not null, or else rename to `result_out_or_null`.

Thanks for the update ✔

> src/hotspot/share/opto/loopPredicate.cpp line 1561:
> 
>> 1559: 
>> 1560: bool PhaseIdealLoop::is_uncommon_trap_if_pattern(IfProjNode* proj) {
>> 1561:   if (proj->is_uncommon_trap_if_pattern()) {
> 
> Is having two methods with an identical name but subtly different semantics not quite confusing, and maybe going to lead to some subtle bugs later on?

Thanks for the update ✔

> src/hotspot/share/opto/subnode.hpp line 300:
> 
>> 298: };
>> 299: 
>> 300: // Does a ScopedValue.get() hits in the cache?
> 
> I finally reconstructed what this node is, and please add a comment saying something like this:
> This node returns true iff this gets us a cache hit (cache reference not null, and at least one of the indices leads to a hit).
> It is essencially a Cmp, comparing the `cache_adr` (you name it scoped_value_cache) with a nullptr.
> But it also gets 2 indices to that cache (ints), which will either score a hit or miss.

Thanks for the update ✔

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482466239
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482466692
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482469006
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482469778
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1482472117