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

Reply via email to