On Thu, 25 May 2023 22:54:15 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 >> 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 of the following. 1) New IR node (suggested by V. >> Kozlov), named SafePointScalarMergeNode, to represent a set of >> SafePointScalarObjectNode; 2) Each scalar replaceable input participating in >> a merge will get a SafePointScalarObjectNode like if it weren't part of a >> merge. 3) Add a new Class to support the rematerialization of SR objects >> that are part of a merge; 4) Patch HotSpot to be able to serialize and >> deserialize debug information related to allocation merges; 5) 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 straightforward: 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 experimented 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 with a new target base due to > a merge or a rebase. The pull request now contains 15 commits: > > - Catching up with master branch. > > Merge remote-tracking branch 'origin/master' into > rematerialization-of-merges > - Address PR review 6: refactoring around rematerialization & improve test > cases. > - Address PR review 5: refactor on rematerialization & add tests. > - Merge remote-tracking branch 'origin/master' into > rematerialization-of-merges > - Address part of PR review 4 & fix a bug setting only_candidate > - Catching up with master > > Merge remote-tracking branch 'origin/master' into > rematerialization-of-merges > - Fix tests. Remember previous reducible Phis. > - Address PR review 3. Some comments and be able to abort compilation. > - Merge with Master > - Addressing PR review 2: refactor & reuse MacroExpand::scalar_replacement > method. > - ... and 5 more: https://git.openjdk.org/jdk/compare/46c4da7f...8f81a7c8 src/hotspot/share/code/debugInfo.cpp line 251: > 249: // Set it to true so that the object will get rematerialized > 250: if (!_selected->is_root()) { > 251: _selected->set_root(true); Why do you need `_selected` to be marked as root? src/hotspot/share/code/debugInfo.cpp line 301: > 299: void ObjectMergeValue::print_detailed(outputStream* st) const { > 300: st->print("merge: ID=%d", _id); > 301: #ifndef PRODUCT Can you post a sample of the output, please? Why is it limited to non-product builds? It's valuable irrespective of build flavor. As I see in `ObjectValue::print_on` and `ScopeDesc::print_on`, you mix `print_on` with `print_fields_on`. Any particular reason for that? You could add `is_object_merge` case in ObjectValue::print_on` instead and extend `ObjectValue::print_fields_on` to cover `ObjectMergeValue` case. I find it hard to reason about `ObjectValue::print_on` vs `ObjectMergeValue::print_on` since it's a non-virtual method. Also, formatting is broken. src/hotspot/share/opto/compile.cpp line 2332: > 2330: } > 2331: > 2332: NOT_PRODUCT(ConnectionGraph::verify_ram_nodes(this, root());) Why do you limit the check to non-product builds only? It won't fail the compilation with product builds. src/hotspot/share/opto/output.cpp line 1101: > 1099: > 1100: if (!is_root) { > 1101: for (int k = 0; k < monarray->length(); k++) { I suggest to turn the lookup over `monarray` into a helper method and call it along with `locarray` and `exparray` checks: bool is_root = locarray->contains(ov) || exparray->contains(ov) || contains_as_owner(monarray, ov); ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1217488199 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1218419279 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1217491794 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1218431285
