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

[Emanuel Peter]

On Thu, 18 Jan 2024 08:36:58 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/loopPredicate.cpp line 1572:
> 
>> 1570: 
>> 1571: 
>> 1572: bool PhaseIdealLoop::loop_predication_for_scoped_value_get(IdealLoopTree* loop, IfProjNode* if_success_proj,
> 
> Add a short comment above, that we are trying to hoist the `If` for a `ScopedValueGetHitsInCache` out of the loop, if possible.
> You have one on the first line, but I think generally we place them at the top, right?

Well, now looking at the method... maybe a longer comment with a picture or pseudocode would be helpful.
It would greatly help me in reviewing the code - otherwise I basically have to draw the picture on a piece of paper myself before understanding it ;)

> src/hotspot/share/opto/loopPredicate.cpp line 1579:
> 
>> 1577:   BoolNode* bol = iff->in(1)->as_Bool();
>> 1578:   if (bol->in(1)->Opcode() == Op_ScopedValueGetHitsInCache && invar.is_invariant(((ScopedValueGetHitsInCacheNode*)bol->in(1))->scoped_value()) &&
>> 1579:       invar.is_invariant(((ScopedValueGetHitsInCacheNode*)bol->in(1))->index1()) && invar.is_invariant(((ScopedValueGetHitsInCacheNode*)bol->in(1))->index2())) {
> 
> Please refactor this if:
> - if we don't take it, you return false, so make it a bailout. This allows you to already bailout if `bol->in(1)->Opcode() == Op_ScopedValueGetHitsInCache` fails.
> - After that check, you can already have this line: `ScopedValueGetHitsInCacheNode* hits_in_the_cache = (ScopedValueGetHitsInCacheNode*) bol->in(1);`, and simplify the other 3 conditions of the if, make it more readable.
> - But make them bailouts as well, instead of indenting the rest of the function.

I don't really know how `is_invar` works. But why is a use-node not automatically variant, if a def-node is variant. Or stated in other terms: why not just check `invar.is_invariant(hits_in_the_cache)`?

> src/hotspot/share/opto/loopnode.cpp line 4737:
> 
>> 4735:     Node* n = _scoped_value_get_nodes.pop();
>> 4736:     assert (n->Opcode() == Op_ScopedValueGetHitsInCache, "");
>> 4737:     ScopedValueGetHitsInCacheNode* get_from_cache = (ScopedValueGetHitsInCacheNode*) n;
> 
> It would be nice to have some variable consistency. Elsewhere you use `sv_hits_in_cache`.
> The name here suggest that this node actually "gets" something from the cache, but it only checks if we have a hit, and another node does the "getting", right?

These are the different names you currently use for `ScopedValueGetHitsInCacheNode`:

sv_hits_in_cache
hits_in_cache
hits_in_the_cache
get_from_cache
get_from_sv_cache
get_from_sv_cache_dom

And for `ScopedValueGetResultNode`:

sv_get_result
get_result
sv_get_result_dom

And for `ScopedValueGetLoadFromCacheNode`:

get_from_cache
load_from_cache
load_from_cache_dom

The names even overlap, e.g. `get_from_cache`.
It would be nice if there was just one name per class, that would enhance the clarity of the code.

> src/hotspot/share/opto/loopnode.cpp line 4745:
> 
>> 4743: }
>> 4744: 
>> 4745: void PhaseIdealLoop::expand_get_from_sv_cache(ScopedValueGetHitsInCacheNode* get_from_cache) {
> 
> Suggestion:
> 
> void PhaseIdealLoop::expand_sv_get_hits_in_cache_and_load_from_cache(ScopedValueGetHitsInCacheNode* get_from_cache) {

And again, a picture / pseudocode of the transformation would help immensely.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1457111178
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1457109943
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1457179839
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1457253594