On Sat, 22 Apr 2023 01:42:41 GMT, Vladimir Ivanov <vliva...@openjdk.org> wrote:
>> 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 > > 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? Hi @iwanowww . I finished implementing a version of this like the illustration below (I didn't add a Candidate class). ScopeValue ObjectValue ObjectAllocationValue AutoBoxObjectValue ObjectMergeValue Here are some observations: - I don't think ObjectMergeValue should be under ObjectValue. The two classes only have two fields in common (_id and _visited). I think it should be a subclass of ScopeValue. - ObjectCandidateValue would need to go under ObjectAllocationValue because it essentially _is_ an ObjectAllocationValue in most aspects. - I didn't add a ObjectCandidateValue class because that class would need to go under ObjectAllocationValue and we would still need to do an "is_object_candidate" before all "is_object_allocation" and we would end up in much the situation that we want to avoid - needing to do is_object_merge before is_object. - It seems the best place to flag an object as candidate is really in ObjectAllocationValue. What do you think? As I said, I already have the code, if you want I can push it and you take a look. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1179649780