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

[Dean Long]

On Fri, 19 Apr 2024 07:49:13 GMT, Galder Zamarreño <galder at openjdk.org> wrote:

>> src/hotspot/share/c1/c1_GraphBuilder.cpp line 4441:
>> 
>>> 4439: 
>>> 4440: void GraphBuilder::append_alloc_array_copy(ciMethod* callee) {
>>> 4441:   {
>> 
>> There are stylistic questions to this block
>> - why separate block?
>> - not necessary comment "Peek at receiver"
>> - misprint in "not primtive array" comment
>> - extra line for {
>> - excessive comment "// not primitive array"
>> - bailout message do not mention phi
>> - please use INLINE_BAILOUT macro
>> 
>> Comment about phi does not make things clear: not evident why receiver type can be nullptr and why it is phi. After all, why can't we check src->exact_type()->as_array_klass()->element_type() here?
>
>> There are stylistic questions to this block
> ...
>> Comment about phi does not make things clear: not evident why receiver type can be nullptr and why it is phi. After all, why can't we check src->exact_type()->as_array_klass()->element_type() here?
> 
> This block comes from @dean-long, I'll let him comment.

> why separate block?

It's left over from when I had it wrapped with `if (UseNewCode)` as I was testing it.  It can be removed.

> not necessary comment "Peek at receiver"

OK.

> misprint in "not primtive array" comment

Good catch.  You can include the suggested fix using the github UI and get added as a contributor if you like.

> extra line for {

To me it makes it more readable, but if it goes against the style guide, feel free to change it.

> excessive comment "// not primitive array"

OK.

> bailout message do not mention phi

Yes, if we bailout due to `receiver_type == nullptr` then we should have a better bailout message.  But you may be correct that it is impossible.  This code was copied from an earlier version where it did seem possible.  If we change it we'll need a new round of testing.

> please use INLINE_BAILOUT macro

We would need a version of INLINE_BAILOUT that doesn't return false.

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

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