On Wed, 5 Apr 2023 16:31: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 with a new target base due to > a merge or a rebase. The pull request now contains seven commits: > > - Merge with Master > - Addressing PR review 2: refactor & reuse MacroExpand::scalar_replacement > method. > - Address PR feeedback 1: make ObjectMergeValue subclass of ObjectValue & > create new IR class to represent scalarized merges. > - Add support for SR'ing some inputs of merges used for field loads > - Fix some typos and do some small refactorings. > - Merge master > - Add support for rematerializing scalar replaced objects participating in > allocation merges Thank you for adding new node - it is more clear now. src/hotspot/share/opto/callnode.hpp line 540: > 538: > 539: bool is_only_merge_sr_candidate() { return > _only_merge_sr_candidate; } > 540: void set_only_merge_sr_candidate(bool only) { _only_merge_sr_candidate > = only; } May be drop `_sr` from names. `SafePointScalarObjectNode` already represents scalarized object. src/hotspot/share/opto/escape.cpp line 633: > 631: > 632: SafePointScalarMergeNode* smerge = new > SafePointScalarMergeNode(merge_t, merge_idx); > 633: smerge->init_req(0, _compile->root()); May be use ophi's control here, it should stay bellow merge point. Was there a reason you use `root`? src/hotspot/share/opto/escape.cpp line 640: > 638: > 639: // Add the selector so we know which direction the execution took > 640: sfpt->add_req(selector); May be added comment that we adding debug info for merge point here (2 values described in the comment for `_merge_pointer_idx`). src/hotspot/share/opto/escape.cpp line 655: > 653: SafePointScalarObjectNode* sobj = > mexp.create_scalarized_object_description(alloc, sfpt); > 654: if (sobj == nullptr) { > 655: fatal("Failed to create SafePointScalarObjectNode!"); This is brutal! May be exit this compilation and recompile without `ReduceAllocationMerges`. src/hotspot/share/opto/escape.cpp line 658: > 656: } > 657: > 658: jvms->set_endoff(sfpt->req()); add comment explaining this line src/hotspot/share/opto/escape.cpp line 677: > 675: > 676: // Replaces debug information references to "ophi" in "sfpt" with > references to "smerge" > 677: int debug_end = jvms->debug_end(); May be add comment that debug info changed (and `debug_end`) due to added scalarized objects info. src/hotspot/share/opto/escape.cpp line 679: > 677: int debug_end = jvms->debug_end(); > 678: sfpt->replace_edges_in_range(ophi, smerge, debug_start, debug_end, > _igvn); > 679: sfpt->set_req(smerge->merge_pointer_idx(jvms), ophi); So you trying to restore `ophi` in debug info which was added at line 637 but then in previous line may be replaced with `smerge`. May add comment explaining that. src/hotspot/share/opto/output.cpp line 755: > 753: ciKlass* cik = t->is_oopptr()->exact_klass(); > 754: assert(cik->is_instance_klass() || > 755: cik->is_array_klass(), "Not supported allocation."); Why spacing changed? src/hotspot/share/opto/output.cpp line 789: > 787: > 788: for (uint i = 1; i < smerge->req(); i++) { > 789: Node* fld_node = smerge->in(i); It is not `fld_node` but `obj_node`. ------------- PR Review: https://git.openjdk.org/jdk/pull/12897#pullrequestreview-1374000788 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1159249159 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1159245961 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1159246463 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1159255417 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1159253457 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1159256643 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1159270793 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1159272308 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1159271887
