RFR: JDK-8316991: Reduce nullable allocation merges [v9]

[Vladimir Ivanov]

On Tue, 26 Mar 2024 19:01:55 GMT, Cesar Soares Lucas <cslucas at openjdk.org> wrote:

>> ### Description
>> 
>> Many, if not most, allocation merges (Phis) are nullable because they join object allocations with "NULL", or objects returned from method calls, etc. Please review this Pull Request that improves Reduce Allocation Merge implementation so that it can reduce at least some of these allocation merges.
>> 
>> Overall, the improvements are related to 1) making rematerialization of merges able to represent "NULL" objects, and 2) being able to reduce merges used by CmpP/N and CastPP. 
>> 
>> The approach to reducing CmpP/N and CastPP is pretty similar to that used in the `MemNode::split_through_phi` method: a clone of the node being split is added on each input of the Phi. I make use of `optimize_ptr_compare` and some type information to remove redundant CmpP and CastPP nodes. I added a bunch of ASCII diagrams illustrating what some of the more important methods are doing.
>> 
>> ### Benchmarking
>> 
>> **Note:** In some of these tests no reduction happens. I left them in to validate that no perf. regression happens in that case.
>> **Note 2:** Marging of error was negligible.
>> 
>> | Benchmark                            |  No RAM (ms/op)  |   Yes RAM (ms/op) |
>> |--------------------------------------|------------------|-------------------|
>> | TestTrapAfterMerge                   |      19.515      |      13.386       |
>> | TestArgEscape                        |      33.165      |      33.254       |
>> | TestCallTwoSide                      |      70.547      |      69.427       |
>> | TestCmpAfterMerge                    |      16.400      |       2.984       |
>> | TestCmpMergeWithNull_Second          |      27.204      |      27.293       |
>> | TestCmpMergeWithNull                 |       8.248      |       4.920       |
>> | TestCondAfterMergeWithAllocate       |      12.890      |       5.252       |
>> | TestCondAfterMergeWithNull           |       6.265      |       5.078       |
>> | TestCondLoadAfterMerge               |      12.713      |       5.163       |
>> | TestConsecutiveSimpleMerge           |      30.863      |       4.068       |
>> | TestDoubleIfElseMerge                |      16.069      |       2.444       |
>> | TestEscapeInCallAfterMerge           |      23.111      |      22.924       |
>> | TestGlobalEscape                     |      14.459      |      14.425       |
>> | TestIfElseInLoop                     |     246.061      |      42.786       |
>> | TestLoadAfterLoopAlias               |      45.808      |      45.812       |
>> ...
>
> Cesar Soares Lucas has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 13 commits:
> 
>  - Merge remote-tracking branch 'origin/master' into ram-nullables
>  - Catching up with master
>  - Fix broken build.
>  - Merge with origin/master
>  - Update test/micro/org/openjdk/bench/vm/compiler/AllocationMerges.java
>    
>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>  - Ammend previous fix & add repro tests.
>  - Fix to prevent reducing already reduced Phi
>  - Fix to prevent creating NULL ConNKlass constants.
>  - Refrain from RAM of arrays and Phis controlled by Loop nodes.
>  - Fix typo in test.
>  - ... and 3 more: https://git.openjdk.org/jdk/compare/89e0889a...3129378f

Overall, looks good. And a nice set of test cases! 

Just minor stylistic comments.

src/hotspot/share/code/debugInfo.hpp line 136:

> 134:   Handle                     _value;
> 135:   bool                       _visited;
> 136:   bool                       _was_scalar_replaced;     // Whether this ObjectValue describes an object scalar replaced or just

Name it `is_scalar_replaced` maybe?

src/hotspot/share/opto/escape.cpp line 472:

> 470: 
> 471:       // Don't handle arrays.
> 472:       if (alloc->Opcode() != Op_Allocate) {

Turn it into `alloc->Opcode() == Op_AllocateArray` or put `assert(alloc->Opcode() == Op_AllocateArray, "")` inside the branch.

src/hotspot/share/opto/escape.cpp line 2976:

> 2974:     //
> 2975:     if (field->base_count() > 1 && candidates.size() == 0) {
> 2976:       bool further_validate = false;

A better name maybe? You can also extract its computation into a helper method (e.g. `has_non_reducible_merge(FieldNode* field)`.

src/hotspot/share/opto/memnode.cpp line 1552:

> 1550: // by 'split_through_phi'. The first use of this method was in EA code as part
> 1551: // of simplification of allocation merges.
> 1552: // Some differences from original method:

What "original method" do you refer to here?

src/hotspot/share/opto/memnode.cpp line 1559:

> 1557:   intptr_t ignore  = 0;
> 1558:   Node*    base    = AddPNode::Ideal_base_and_offset(address, phase, ignore);
> 1559:            base    = (base->is_CastPP()) ? base->in(1) : base;

With proposed identation It's hard to read. I'd go even further and turn it into an if:

if (base->is_CastPP()) {
  base = base->in(1);
}

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

Marked as reviewed by vlivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15825#pullrequestreview-1961704899
PR Review Comment: https://git.openjdk.org/jdk/pull/15825#discussion_r1540097586
PR Review Comment: https://git.openjdk.org/jdk/pull/15825#discussion_r1540096986
PR Review Comment: https://git.openjdk.org/jdk/pull/15825#discussion_r1540093774
PR Review Comment: https://git.openjdk.org/jdk/pull/15825#discussion_r1541553698
PR Review Comment: https://git.openjdk.org/jdk/pull/15825#discussion_r1540092689