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

[Per Liden]

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
>>>>