RFR: JDK-8287061: Support for rematerializing scalar replaced objects participating in allocation merges [v4]
Vladimir Kozlov
kvn at openjdk.org
Fri Mar 24 19:46:36 UTC 2023
On Mon, 20 Mar 2023 19:23:34 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 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
More information about the security-dev
mailing list