On Thu, 30 Mar 2023 23:36:20 GMT, Cesar Soares Lucas <cslu...@openjdk.org> wrote:
>> Can I please get reviews for this PR? >> >> The most common and frequent use of NonEscaping Phis merging object >> allocations is for debugging information. The two graphs below show numbers >> for Renaissance and DaCapo benchmarks - similar results are obtained for all >> other applications that I tested. >> >> With what frequency does each IR node type occurs as an allocation merge >> user? I.e., if the same node type uses a Phi N times the counter is >> incremented by N: >> >>  >> >> What are the most common users of allocation merges? I.e., if the same node >> type uses a Phi N times the counter is incremented by 1: >> >>  >> >> This PR adds support scalar replacing allocations participating in merges >> that are used as debug information OR as a base for field loads. I plan to >> create subsequent PRs to enable scalar replacement of merges used by other >> node types (CmpP is next on the list) subsequently. >> >> The approach I used for _rematerialization_ is pretty straightforward. It >> consists basically in: 1) Extend SafePointScalarObjectNode to represent >> multiple SR objects; 2) Add a new Class to support rematerialization of SR >> objects part of merges; 3) Patch HotSpot to be able to serialize and >> deserialize debug information related to allocation merges; 4) Patch C2 to >> generate unique types for SR objects participating in some allocation merges. >> >> The approach I used for _enabling the scalar replacement of some of the >> inputs of the allocation merge_ is also pretty straight forward: call >> `MemNode::split_through_phi` to, well, split AddP->Load* through the merge >> which will render the Phi useless. >> >> I tested this with JTREG tests tier 1-4 (Windows, Linux, and Mac) and didn't >> see regression. I also tested with several applications and didn't see any >> failure. I also ran tests with "-ea -esa -Xbatch -Xcomp >> -XX:+UnlockExperimentalVMOptions -XX:-TieredCompilation -server >> -XX:+IgnoreUnrecognizedVMOptions -XX:+UnlockDiagnosticVMOptions >> -XX:+StressLCM -XX:+StressGCM -XX:+StressCCP" and didn't observe any related >> failures. > > Cesar Soares Lucas has updated the pull request incrementally with one > additional commit since the last revision: > > Address PR feeedback 1: make ObjectMergeValue subclass of ObjectValue & > create new IR class to represent scalarized merges. src/hotspot/share/opto/macro.cpp line 632: > 630: safepoints->append_if_missing(sfpt); > 631: } > 632: } else if (ignore_merges && (use->is_Phi() || use->is_EncodeP() || > use->Opcode() == Op_MemBarRelease)) { I try to understand this part. now `can_eliminate_allocation` can pre-test whether SR can eliminate the alloc. I see that you use it in EA. With ignore_merges, why we also skip EncodeP or MemBarRelease here? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1154771471
