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

[Vladimir Kozlov]

On Wed, 31 Aug 2022 20:00:04 GMT, Cesar Soares <duke at openjdk.org> wrote:

>>> This is good starting point. To have new "Phi" type node to collect information about merged allocation.
>>> I need more time to dive into changes to give review.
>> 
>> Thanks for taking the time to look into this!
>> 
>>> I currently found one issue we need discuss - merge allocation of different subclasses (of the same parent class) which may have different number of fields. Current implementation assume objects are the same but I don't see the check for it during RAM node creation. May be we should have it at this initial implementation.
>> 
>> Got it. I'll create some tests for this and see what happens.
>> 
>>> What about adjust_scalar_replaceable_state() code mark allocation as non-SR if they are merged?
>> 
>> I didn't get this part. Can you please clarify?
>> 
>>> About input memory slices. Since merged allocation are SR we should have some new memory Phi created in EA split_memory_phi() which we can try to identify instead of adding all memory slices we find (I am talking about RAM constructor).
>> 
>> I'll take a look into that. Thanks for the suggestion.
>> 
>>> I see you bailed compilation to recompile in case you can't remove RAM node. I think it is fine for initial implementation.
>> 
>> Great.
>
>>I currently found one issue we need discuss - merge allocation of different subclasses (of the same parent class) which may have different number of fields. Current implementation assume objects are the same but I don't see the check for it during RAM node creation. May be we should have it at this initial implementation.
> 
> Hi again @vnkozlov - The current implementation requires the Klass of all inputs to the Phi to be the same. I'm trying to lift that restriction as it would increase the number of cases where the merge can be removed. I've been trying to create an example where I'd need to enforce the allocations to be of the same type but I couldn't figure one out yet. Do you have an example where that must be enforced? Note that I only need fields accessed through the merge Phi node, which means only fields present in the superclass AFAIU.

Hi @JohnTortugo 
Yes, I think you can lift restriction for all inputs have the same class. I looked more and you correctly set RAM's type to original Phi's type - it should be correct type (base class, which could be also one of inputs). You correctly register fields offsets based on this (Phi) class during RAM make.  C2 type system and bytecode should guarantee that Phi's type is correct (meet of all inputs). It could be more narrowed during CCP phase but type you got serves correctly for this implementation. May be assert that it is not interface - or skip such cases if you find them.

You can add `assert(input_t->is_instptr()->instance_klass()->is_subtype_of(klass))` instead of klass equals check.

What confused me is code in `PhaseMacroExpand::scalar_replacement()` where you process Allocation from one of inputs which may have more fields than RAM registered. But you correctly have `!ram->needs_field(offset)` check which I missed before. That why I commented.

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

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