RFR(S): ZGC: C2: Oop instance cloning causing skipped compiles

[Nils Eliasson]

Thank you Per!

// Nils

On 2019-11-28 12:10, Per Liden wrote:
> Thanks Nils!
>
> Looks good to me.
>
> /Per
>
> On 11/27/19 11:50 AM, Nils Eliasson wrote:
>> Hi Per,
>>
>> Here is an update webrev with your fixes included:
>>
>> http://cr.openjdk.java.net/~neliasso/8234520/webrev.05/
>>
>> I chose to go with CloneInst, and ClonePrimArray.
>>
>> CopyOf, CopyOfRange - like ArrayCopy, doesn't have specialized 
>> versions, rather check the type when expanding, and of course - there 
>> are no versions for instances. But there are differences to what 
>> guards they need, and when the guards are expanded. I would need to 
>> dig down into the details to determine if it could be simplified.
>>
>> Regards,
>>
>> Nils
>>
>> On 2019-11-26 14:29, Per Liden wrote:
>>> Hi Nils,
>>>
>>> On 11/21/19 12:53 PM, Nils Eliasson wrote:
>>>> I updated this to version 2.
>>>>
>>>> http://cr.openjdk.java.net/~neliasso/8234520/webrev.02/
>>>>
>>>> I found a problen running 
>>>> compiler/arguments/TestStressReflectiveCode.java
>>>>
>>>> Even though the clone was created as a oop clone, the type node 
>>>> type returns isa_aryprt. This is caused by the src ptr not being 
>>>> the base pointer. Until I fix that I wanted a more robust test.
>>>>
>>>> In this webrev I split up the is_clonebasic into is_clone_oop and 
>>>> is_clone_array. (is_clone_oop_array is already there). Having a 
>>>> complete set with the three clone types allows for a robust test 
>>>> and easy verification. (The three variants end up in different 
>>>> paths with different GCs).
>>>
>>> A couple of suggestions:
>>>
>>> 1) Instead of
>>>
>>>  CloneOop
>>>  CloneArray
>>>  CloneOopArray
>>>
>>> I think we should call the three types:
>>>
>>>  CloneInstance
>>>  CloneTypeArray
>>>  CloneOopArray
>>>
>>> Since CloneOop is not actually cloning an oop, but an 
>>> object/instance. And being explicit about TypeArray seems like a 
>>> good thing to avoid any confusion about the difference compared to 
>>> CloneOopArray. I guess PrimArray would be an alternative to TypeArray.
>>>
>>> And of course, if we change this then the is_clone/set_clone 
>>> functions should follow the same naming convention.
>>>
>>> Btw, what about CopyOf and CopyOfRange? Don't they also come in Oop 
>>> and Type versions, or are we handling those differently in some way? 
>>> Looking at the code it looks like they are only used for the oop 
>>> array case?
>>
>>>
>>>
>>> 2) In zBarrierSerC2.cpp, do you mind if we do like this instead? I 
>>> find that quite a bit easier to read.
>>>
>>> [...]
>>>   const Type** domain_fields = TypeTuple::fields(4);
>>>   domain_fields[TypeFunc::Parms + 0] = TypeInstPtr::NOTNULL; // src
>>>   domain_fields[TypeFunc::Parms + 1] = TypeInstPtr::NOTNULL; // dst
>>>   domain_fields[TypeFunc::Parms + 2] = TypeLong::LONG; // size lower
>>>   domain_fields[TypeFunc::Parms + 3] = Type::HALF; // size upper
>>>   const TypeTuple* domain = TypeTuple::make(TypeFunc::Parms + 4, 
>>> domain_fields);
>>> [...]
>>>
>>>
>>> 3) I'd also like to add some const, adjust indentation, etc, in a 
>>> few places. Instead of listing them here I made a patch, which goes 
>>> on top of yours. This patch also adjusts 2) above. Just shout if you 
>>> have any objections.
>>>
>>> http://cr.openjdk.java.net/~pliden/8234520/webrev.03-review
>>>
>>> /Per
>>>
>>>>
>>>> Regards,
>>>>
>>>> Nils
>>>>
>>>>
>>>> On 2019-11-20 15:25, Nils Eliasson wrote:
>>>>> Hi,
>>>>>
>>>>> I found a few bugs after the enabling of the clone intrinsic in ZGC.
>>>>>
>>>>> 1) The arraycopy clone_basic has the parameters adjusted to work 
>>>>> as a memcopy. For an oop the src is pointing inside the oop to 
>>>>> where we want to start copying. But when we want to do a runtime 
>>>>> call to clone - the parameters are supposed to be the actual src 
>>>>> oop and dst oop, and the size should be the instance size.
>>>>>
>>>>> For now I have made a workaround. What should be done later is 
>>>>> using the offset in the arraycopy node to encode where the payload 
>>>>> is, so that the base pointers are always correct. But that would 
>>>>> require changes to the BarrierSet classes of all GCs. So I leave 
>>>>> that for next release.
>>>>>
>>>>> 2) The size parameter of the TypeFunc for the runtime call has the 
>>>>> wrong type. It was originally Long but missed the upper Half, it 
>>>>> was fixed to INT (JDK-8233834), but that is wrong and causes the 
>>>>> compiles to be skipped. We didn't notice that since they failed 
>>>>> silently. That is also why we didn't notice problem #1 too.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8234520
>>>>>
>>>>> http://cr.openjdk.java.net/~neliasso/8234520/webrev.01/
>>>>>
>>>>> Please review!
>>>>>
>>>>> Nils
>>>>>