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

[Per Liden]

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