RFR: 8302850: Implement C1 clone intrinsic that reuses arraycopy code for primitive arrays [v7]

[Dean Long]

On Mon, 25 Mar 2024 09:49:12 GMT, Galder Zamarreño <galder at openjdk.org> wrote:

>> src/hotspot/share/c1/c1_GraphBuilder.cpp line 2146:
>> 
>>> 2144:       ciType* receiver_type;
>>> 2145:       if (target->get_Method()->intrinsic_id() == vmIntrinsics::_clone &&
>>> 2146:           ((receiver_type = state()->stack_at(state()->stack_size() - inline_target->arg_size())->exact_type()) == nullptr || // clone target is phi
>> 
>> I don't think target-specific logic belongs here.  And I don't understand the point about Phi nodes.  Isn't the holder_known flag enough?  For primitive arrays, isn't it true that inline_target->get_Method()->intrinsic_id() == vmIntrinsics::_clone?
>
>> I don't think target-specific logic belongs here. And I don't understand the point about Phi nodes. Isn't the holder_known flag enough?
> 
> In my testing `holder_known` was not enough to detect objects that are not Phi. For example:
> 
> 
>     static int[] test(int[] ints)
>     {
>         return ints.clone();
>     }
> 
> 
> `holder_known` is false when it tries to C1 compile `ints.clone()`, am I missing something here?
> 
>> For primitive arrays, isn't it true that inline_target->get_Method()->intrinsic_id() == vmIntrinsics::_clone?
> 
> Possibly, but in this part of the logic I'm trying to find situations in which I don't want to apply the `clone` intrinsic. And those situations are non-array objects, and for arrays, those whose elements are not primitives. I don't see how I can craft such a condition with only `inline_target->get_Method()->intrinsic_id() == vmIntrinsics::_clone`? IOW, that condition might be true for primitive arrays, but is it false for non-array objects and non-primitive arrays?

You're right about holder_known, but why do you need to check for _clone specifically at line 2137?  If there is logic missing that prevents an inlining attempt then I think it should be fixed first, rather than in a followup.

And I see that you need to do a receiver type check to allow only primitive arrays.  Can you do that in append_alloc_array_copy, and bailout if not successful?  The logic in build_graph_for_intrinsic would need to change slightly to support this.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17667#discussion_r1540085587