Plan to create unique types for inputs of allocation merges

Tue Mar 7 02:01:21 UTC 2023

Hi, Vladimir.

Once more, thank you for taking a look at my proposal and reviewing my PRs.

> I'm glad you continue working on the EA enhancement. Looking forward for
> the next iteration of your work!

I just created a new PR (https://github.com/openjdk/jdk/pull/12897) with the 
latest progress I have on this front! This latest PR adds support for re-materialization
of scalar replaced objects participating in merges. I.e., the `new Point` below 
will now be scalar replaced. Note that only one of the inputs to the
merge is scalar replaced.

Point p = new Point(...);
if (...)
    p = otherMethod(...);
new Unloaded();

The current PR only handle merges that are used solely as debug information.

The next PR I'm going to create is for scalar replacing merges that have field 
loads as users and I'm still considering using LoadNode::split_through_phi for that.
However, I'm still investigating if I'll be able to use split_through_phi since it
requires `instance_id`.

> I still think that improving split_unique_types to handle allocation
> merges is a better alternative to custom RAM nodes with an extra phase
> to optimize them.

I agree with you here. I don't think I'll need new IR node for untangling the merges.

> I wouldn't be bothered too much by the fact that stores can't be
> reliably optimized in general case. The whole idea behind JDK-8289943 is
> to focus on "simple enough" shapes. There will always be complex enough
> code shapes the conservative analysis couldn't handle.

Sounds good to me!

Cheers,
Cesar
________________________________________
From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
Sent: Monday, February 27, 2023 2:01 PM
To: Cesar Soares Lucas; hotspot-compiler-dev at openjdk.java.net
Subject: Re: Plan to create unique types for inputs of allocation merges

Hi Cesar,

I assume you consider enhancing split_unique_types as part of
JDK-8289943 [1] we discussed before.

I still think that improving split_unique_types to handle allocation
merges is a better alternative to custom RAM nodes with an extra phase
to optimize them.

Your proposal looks reasonable.

Speaking of Store nodes, my recollection is they aren't handled by
pr/9073 [1] yet. (In particular, ConnectionGraph::can_reduce_this_phi()
bails on !ConnectionGraph::is_read_only() check [2].)

I wouldn't be bothered too much by the fact that stores can't be
reliably optimized in general case. The whole idea behind JDK-8289943 is
to focus on "simple enough" shapes. There will always be complex enough
code shapes the conservative analysis couldn't handle.

I'm glad you continue working on the EA enhancement. Looking forward for
the next iteration of your work!

Best regards,
Vladimir Ivanov

[1] https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.openjdk.org%2Fjdk%2Fpull%2F9073&data=05%7C01%7CDivino.Cesar%40microsoft.com%7Cc583064e305e4368c6a408db190e3c98%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638131321163410389%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=uO1mpEkUQiupJ5TtMUGitwwcq%2BzQe1FKg6lKqJOsc5U%3D&reserved=0
[2]
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenjdk%2Fjdk%2Fpull%2F9073%2Ffiles%23diff-03f7ae3cf79ff61be6e4f0590b7809a87825b073341fdbfcf36143b99c304474R652&data=05%7C01%7CDivino.Cesar%40microsoft.com%7Cc583064e305e4368c6a408db190e3c98%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638131321163410389%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=JOXdGfitlt84KXZNYYLC%2BEyZrZNR1N8N0SAMoh53vlA%3D&reserved=0

On 2/6/23 11:56, Cesar Soares Lucas wrote:
> Hello there!
>
> Can I please get some feedback on the following plan?
>
> ---
>
> Plan to patch the split_unique_types method to make it able to create
>
> unique types for inputs participating in NoEscape merges. The motivation for
>
> doing this is because it will make possible to remove the allocation merge
>
> (after making other changes) and scalar replace some of the merge's input. I
>
> will be concurrently working on a different set of changes to eliminate the
>
> allocation merges.
>
> My idea is to approach this in three parts. The parts are separated by
> the type
>
> of node that uses the allocation merge.
>
> 1) Merges that have only SafePoint (and subclasses) nodes as users. This
> will
>
> let me handle merges used as debug information in SafePoints. There are
> not many
>
> merges that are only used by SafePoints but SafePointNode is the most common
>
> user of merges (the merge is used by a SafePointNode and some other type
>
> of node).
>
> 2) Also handle merges that have field loads as users. This will let me
> later on
>
> split-thru-phi the loads and after some other changes remove the merge.
>
> With (1) and (2) in place many (maybe most) merge usage patterns can be
>
> simplified.
>
> 3) Also handle merges that have Store as users. This is by far the most
>
> complicated case. It might not be possible to handle this kind of merge in
>
> general, but we can make some cases work. My current idea for handling this
>
> scenario is to clone the Store, make each one use a different input from the
>
> merge and have a different unique type. This will require adding a "selector
>
> phi" to output which input of the merge should be used and IfNode's to
> control
>
> the execution of the cloned Store's. I understand we'll need to limit
> the number
>
> of bases and stores that we can handle.
>
> As an illustration of the 3rd scenario above, this code:
>
> p = phi(x, o1, o2);
>
> p.x = 10;           // no instance_id
>
> would become:
>
> p1 = phi(x, o1, NULL);
>
> p2 = phi(x, NULL, o2);
>
> sl = phi(x, 0, 1);  // selector Phi
>
> if (sl == 0)
>
>      p1.x = 10;      // instance_id "1"
>
> else
>
>      p2.x = 10;      // instance_id "2"
>
> Best regards,
>
> Cesar
>