On Fri, 24 Mar 2023 19:02:57 GMT, Vladimir Kozlov <k...@openjdk.org> wrote:
>> Cesar Soares Lucas has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add support for SR'ing some inputs of merges used for field loads
>
> src/hotspot/share/code/debugInfo.hpp line 199:
>
>> 197: // ObjectValue describing an object that was scalar replaced.
>> 198:
>> 199: class ObjectMergeValue: public ScopeValue {
>
> Why you did not make subclass of ObjectValue? You would need to check
> `sv->is_object_merge()` first before `sv->is_object()` in few places. But on
> other hand you don't need to duplicates ObjectValue`s fields and asserts.
Let me try that and see how it looks.
> src/hotspot/share/opto/callnode.hpp line 511:
>
>> 509: // by a SafePoint; 2) A scalar replaced object is participating in an
>> allocation
>> 510: // merge (Phi) and the Phi is referenced by a SafePoint. The schematics
>> of how
>> 511: // 'spobj' is used in both scenarios are described below.
>
> I am not comfortable with reusing SafePointScalarObjectNode for 2) since it
> describes totally different information.
> I think it should be separate Node which points to array of SFSO id (in
> addition to Phis) similar how we do now if SFSO is referenced in other SFSO's
> field. SFSO could be created before the merge. Consider:
>
> Point p = new Point();
> Point q = foo();
> if (cond) {
> q = p;
> }
> trap(p, q);
I had considered that but decided not to do it to prevent adding a new IR node.
I'll give that a shot and update this thread with how it goes.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1148150933
PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1148151474
