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

[Vladimir Ivanov]

On Thu, 6 Oct 2022 16:50:28 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:
> 
>   Fix x86 tests.

Very nice work, Cesar. Most notably, I'm happy to see the test with so many non-trivial cases enumerated.

Speaking of the proposed patch itself, I'm not done yet reviewing it.

As of now, I don't fully grasp what's the purpose and motivation to introduce `ReducedAllocationMerge`. I would be grateful for additional information about how you ended up with the current design. (I went through the email thread, but it didn't help me.)

In particular, I still don't understand how it interacts with existing scalar replacement logic when it comes to unique (per-allocation) memory slices.

Also, on bailouts when the new analysis fails: I instrumented all possible failure modes with asserts and all the test failures I saw were in 2 places:

src/hotspot/share/opto/callnode.cpp:1912
    } else if (input->bottom_type()->base() == Type::Memory) {
      // Somehow the base was eliminated and we still have a memory reference left
      assert(false, "");
      return NULL;
    }

src/hotspot/share/opto/macro.cpp:2612
        // In some cases the region controlling the RAM might go away due to some simplification
        // of the IR graph. For now, we'll just bail out if this happens.
        if (n->in(0) == NULL || !n->in(0)->is_Region()) {
          assert(false, "");
          C->record_failure(C2Compiler::retry_no_reduce_allocation_merges());
          return;
        }

How hard would it be to extend the test with cases which demonstrate existing limitations?

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

> 543:   bool split_AddP(Node *addp, Node *base);
> 544: 
> 545:   PhiNode *create_split_phi(PhiNode *orig_phi, int alias_idx, GrowableArray<PhiNode *>  *orig_phi_worklist, bool &new_created);

What's the point of converting `orig_phi_worklist` into a pointer?

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

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