On Mon, 20 Mar 2023 19:23:34 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 run 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: > > Add support for SR'ing some inputs of merges used for field loads Initial review. 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. src/hotspot/share/opto/callnode.cpp line 1479: > 1477: #ifdef ASSERT > 1478: _alloc(alloc), > 1479: #endif May be we should always pass alloc, even in product VM. It is not related to your changes but it is pain to have. 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); src/hotspot/share/opto/callnode.hpp line 519: > 517: // _nfields : how many fields the SR object has. > 518: // _alloc : pointer to the Allocate object that previously > created the SR object. > 519: // Only used for debug purposes. May be useful in other cases too in a future not only in debug. src/hotspot/share/opto/macro.hpp line 196: > 194: Node* size_in_bytes); > 195: > 196: static Node* make_arraycopy_load(Compile* comp, PhaseIterGVN* igvn, > ArrayCopyNode* ac, intptr_t offset, Node* ctl, Node* mem, BasicType ft, const > Type *ftype, AllocateNode *alloc); Why you need this change? It polluted diffs and hide important changes. Could be separate change from this one. ------------- PR Review: https://git.openjdk.org/jdk/pull/12897#pullrequestreview-1357285068 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1147961058 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1147963487 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1147991641 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1147965112 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1147973203
