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

[Nils Eliasson]

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