RFR: 8289943: Simplify some object allocation merges [v10]

[Vladimir Kozlov]

On Wed, 28 Sep 2022 00:32:30 GMT, Cesar Soares Lucas <cslucas at openjdk.org> wrote:

>> Hi there, can I please get some feedback on this approach to simplify object allocation merges in order to promote Scalar Replacement of the objects involved in the merge?
>> 
>> The basic idea for this [approach was discussed in this thread](https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2022-April/055189.html) and it consists of: 
>> 1) Identify Phi nodes that merge object allocations and replace them with a new IR node called ReducedAllocationMergeNode (RAM node).
>> 2) Scalar Replace the incoming allocations to the RAM node.
>> 3) Scalar Replace the RAM node itself.
>> 
>> There are a few conditions for doing the replacement of the Phi by a RAM node though - Although I plan to work on removing them in subsequent PRs:
>> 
>> - The only supported users of the original Phi are AddP->Load, SafePoints/Traps, DecodeN.
>> 
>> These are the critical parts of the implementation and I'd appreciate it very much if you could tell me if what I implemented isn't violating any C2 IR constraints:
>> 
>> - The way I identify/use the memory edges that will be used to find the last stored values to the merged object fields.
>> - The way I check if there is an incoming Allocate node to the original Phi node.
>> - The way I check if there is no store to the merged objects after they are merged.
>> 
>> Testing:
>> - Windows/Linux/MAC fastdebug/release 
>>   - hotspot_all
>>   - tier1
>>   - Renaissance
>>   - dacapo
>>   - new IR-based tests
>
> Cesar Soares Lucas has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Addressing PR feedback. Added new constraint for case of merging SR and NSR allocations.

Thank you for addressing my comments. I have mostly code style related comments left (and default flag's value).
I will submit testing and we will see how it goes.

src/hotspot/share/opto/c2_globals.hpp line 479:

> 477:                                                                             \
> 478:   product(bool, ReduceAllocationMerges, false,                              \
> 479:           "Try to simplify allocation merges before Scalar Replacement")    \

I don't see flag's value change to `true` as suggested.

src/hotspot/share/opto/callnode.cpp line 1780:

> 1778:     }
> 1779:   }
> 1780:   else {

Style - should be one line: ` } else {`.
I noticed that here and in other places in new code you put `else` and `else if (` on separate line from `}`. We use one line in such cases.

src/hotspot/share/opto/callnode.cpp line 1883:

> 1881: 
> 1882:   Node* load = LoadNode::make(*igvn, ctrl, mem, addp, adr_type, field_type, basic_elem_type, MemNode::unordered,
> 1883:                                 LoadNode::DependsOnlyOnTest, false, false, false, false, (uint8_t)0U, false);

Add comment explaining why you want to keep `LoadN` node without `DecodeN`.

src/hotspot/share/opto/escape.hpp line 606:

> 604:   //    - SafePointNode or uncommon traps
> 605:   //    - DecodeN->AddP->Loads
> 606:   bool can_reduce_this_phi(const Node* n) const;

Move detailed description comment to .cpp file.

src/hotspot/share/opto/escape.hpp line 686:

> 684:   // LocalVar(12)  NoEscape(NoEscape) Edges: [ 119 107P       ] Uses: [ ---         ]  124  CheckCastPP ...
> 685:   // LocalVar(11)  NoEscape(NoEscape) Edges: [ 40 28P         ] Uses: [ ---         ]   45  CheckCastPP ...
> 686:   void remove_phi_node(PointsToNode* ptn);

Move detailed description comment to .cpp file.

src/hotspot/share/opto/macro.cpp line 2855:

> 2853:   return total;
> 2854: }
> 2855: #endif

Undo this.

src/hotspot/share/opto/macro.hpp line 119:

> 117:   // 'ram' itself. For those cases we create an SafePointScalarObjectNode,
> 118:   // similar to what is done to regular scalar replacement.
> 119:   bool eliminate_reduced_allocation_merge(ReducedAllocationMergeNode *ram);

Please, move this detailed description comment to `.cpp` file to the method's code. Here 2 lines is enough.

src/hotspot/share/opto/output.cpp line 860:

> 858:     if (sv == NULL) {
> 859:       ciKlass* cik = t->isa_instptr() != NULL ? t->isa_instptr()->instance_klass()
> 860:                                               : t->is_oopptr()->exact_klass();

Why this change? Add comment.

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

PR: https://git.openjdk.org/jdk/pull/9073