RFR: JDK-8287061: Support for rematerializing scalar replaced objects participating in allocation merges [v7]
Vladimir Kozlov
kvn at openjdk.org
Thu Apr 6 04:46:25 UTC 2023
On Wed, 5 Apr 2023 16:31:20 GMT, Cesar Soares Lucas <cslucas at 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
More information about the security-dev
mailing list