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

[Vladimir Ivanov]

> http://cr.openjdk.java.net/~neliasso/8234520/webrev.03/

====================================================================

src/hotspot/share/gc/z/c2/zBarrierSetC2.cpp:

+  // The currently modeled arraycopy-clone_basic doesn't have the base 
pointers for src and dst,
+  // rather point at the start of the payload.
+  Node* src_base = get_base_for_arracycopy_clone(phase, src);
+  Node* dst_base = get_base_for_arracycopy_clone(phase, dst);
+
+  // The size must also be increased to match the instance size.
+  int base_off = BarrierSetC2::extract_base_offset(false);
+  Node* full_size = phase->transform_later(new AddLNode(size, 
phase->longcon(base_off >> LogBytesPerLong)));
    Node* const call = phase->make_leaf_call(ctrl,
                                             mem,
                                             clone_type(),
 
ZBarrierSetRuntime::clone_addr(),
                                             "ZBarrierSetRuntime::clone",
                                             TypeRawPtr::BOTTOM,
-                                           src,
-                                           dst,
-                                           size);
+                                           src_base,
+                                           dst_base,
+                                           full_size,
+                                           phase->top());


Do you see any problems with copying object header?

====================================================================

The rest are minor comments:

src/hotspot/share/gc/shared/c2/barrierSetC2.cpp:

-void BarrierSetC2::clone(GraphKit* kit, Node* src, Node* dst, Node* 
size, bool is_array) const {
-  // Exclude the header but include array length to copy by 8 bytes words.
-  // Can't use base_offset_in_bytes(bt) since basic type is unknown.
+int BarrierSetC2::extract_base_offset(bool is_array) {

...

+void BarrierSetC2::clone(GraphKit* kit, Node* src, Node* dst, Node* 
size, bool is_array) const {
+  // Exclude the header but include array length to copy by 8 bytes words.
+  // Can't use base_offset_in_bytes(bt) since basic type is unknown.
+  int base_off = extract_base_offset(is_array);


I'd leave the comment in BarrierSetC2::extract_base_offset(). After the 
refactoring it looks confusing in BarrierSetC2::clone().

Also, considering it's used from zBarrierSetC2.cpp, it would be nice to 
have a more descriptive name. 
BarrierSetC2::arraycopy_payload_base_offset(bool is_array) maybe?


====================================================================

src/hotspot/share/gc/z/c2/zBarrierSetC2.cpp:

+Node* get_base_for_arracycopy_clone(PhaseMacroExpand* phase, Node* n) {

Should get_base_for_arracycopy_clone be static? Also, would be nice if 
the name reflects that it works on instances and not arrays.

====================================================================

+  // The currently modeled arraycopy-clone_basic doesn't have the base 
pointers for src and dst,
+  // rather point at the start of the payload.
+  Node* src_base = get_base_for_arracycopy_clone(phase, src);
+  Node* dst_base = get_base_for_arracycopy_clone(phase, dst);

Another thing: "base" in zBarrierSetC2.cpp and in BarrierSetC2 has 
opposite meaning which is confusing.

Renaming src/dst<->src_base/dst_base in 
ZBarrierSetC2::clone_at_expansion() would improve things.

====================================================================

-  if (src->bottom_type()->isa_aryptr()) {
+  if (ac->is_clone_array()) {
      // Clone primitive array

Is the comment valid? Doesn't it cover object array case as well?


Best regards,
Vladimir Ivanov

> 
> Regards,
> 
> Nils
> 
> 
> On 2019-11-21 12:53, 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).
>>
>> 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
>>>