On Thu, 20 Apr 2023 19:27:58 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 10 commits: > > - 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. > - 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 Nice work, Cesar! I like how the patch shapes now. I'm not done with the review yet, but decided to share the comments I have so far. src/hotspot/share/code/debugInfo.cpp line 232: > 230: // If we call select again on the same merge we should return the same > result > 231: if (_selected != nullptr) { > 232: return _selected; I'm not sure I understand how it is intended to work. The code below initializes `_selected`, but returns `nullptr` when `selector >= 0`. Subsequent calls will return non-null value. src/hotspot/share/code/debugInfo.cpp line 257: > 255: } else { > 256: assert(selector < _possible_objects.length(), "sanity"); > 257: _selected = (ObjectValue*) _possible_objects.at(selector); Any particular reason to reuse `ObjectValue` from `_possible_objects` instead of allocating a fresh one (as you do on `selector == -1` bracnh)? I'd prefer `ObjectMergeValue::select()` to always allocate a fresh `ObjectValue` when converting `ObjectMergeValue` + `ObjectMergeCandidateValue` into `ObjectValue`. src/hotspot/share/code/debugInfo.hpp line 199: > 197: // ObjectValue describing an object that was scalar replaced. > 198: > 199: class ObjectMergeValue: public ObjectValue { I find the decision to subclass`ObjectValue` confusing and error prone: now `is_object()` returns true for `ObjectMergeValue`, but you have to apply the selector first to turn it into `ObjectValue`. And now the order of checks matter, so you always have to perform `is_object_merge()` first and then follow it with `is_object()` guard. You have 3 flavors of `ObjectValue` now: * good old `ObjectValue`; * `ObjectMergeValue` * merge candidates (`ObjectMergeCandidateValue`?) Does it make sense to introduce 3 different subclasses under `ObjectValue` to clearly distinguish the scenarios? src/hotspot/share/code/debugInfo.hpp line 242: > 240: bool is_cached() const { return > _cached; } > 241: void set_cached(bool cached) { _cached = > cached; } > 242: AutoBoxObjectValue(int id, ScopeValue* klass, bool > only_merge_candidate = false) : ObjectValue(id, klass, only_merge_candidate), > _cached(false) { } Any particular reason to allow `AutoBoxObjectValue` to be a merge candidate? src/hotspot/share/opto/escape.hpp line 593: > 591: // Methods related to Reduce Allocation Merges > 592: > 593: bool can_reduce_this_phi(PhiNode* ophi) const; On naming: IMO referring to "this" doesn't help, but adds noise. If you drop it ("can_reduce_this_phi" => "can_reduce_phi"), it's still clear what the method does. src/java.base/share/classes/java/security/AccessController.java line 786: > 784: // allocation merge Phi leading to it) might become NonEscaping > and get > 785: // scalar replaced. The call below enforces 'result' to always > escape. > 786: ensureMaterializedForStackWalk(result); Why don't you add the same call in the other `executePrivileged` overload? It has the very same code shape. ------------- PR Review: https://git.openjdk.org/jdk/pull/12897#pullrequestreview-1396497913 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1174242946 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1174249820 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1174248472 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1174250881 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1174248735 PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1174235850
