[jdk17] RFR: 8268362: [REDO] C2 crash when compile negative Arrays.copyOf length after loop [v2]

[Hui Shi]

On Thu, 17 Jun 2021 04:07:32 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Hui Shi has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   move duplicate code into GraphKit::cast_replace_array_length_post_allocation & add more comments
>
> src/hotspot/share/opto/graphKit.cpp line 1192:
> 
>> 1190:       _gvn.set_type_bottom(ccast);
>> 1191:       record_for_igvn(ccast);
>> 1192:       alen = ccast;
> 
> I think we need GraphKit wrapper method which execute this code to avoid duplication here, in `arraycopy_move_allocation_here()` and in `new_array()`.
> Add comment why we can't do `transform()`.

@vnkozlov  Thanks for your review!

GraphKit::load_array_length code is different with new_array() and arraycopy_move_allocation_here(), it doesn't replace_in_map. Currently GraphKit::cast_replace_array_length_post_allocation warpper is used in new_array() and arraycopy_move_allocation_here().

Would you prefer a wrapper for all these three places? It would be like following code.

Node* GraphKit::cast_array_length_post_allocation(AllocateArrayNode* alloc, const TypeOopPtr* oop_type, bool replace_map /false in load_array_length/) {
  Node* length = alloc->in(AllocateNode::ALength);
  if (replace_map == false || map()->find_edge(length) >= 0) {
    Node* ccast = alloc->make_ideal_length(oop_type, &_gvn);
    if (ccast != length) {
      _gvn.set_type_bottom(ccast);
      record_for_igvn(ccast);
      if (replace_map) {
        replace_in_map(length, ccast);
      }
      return cast;
    }
  }
  return length;
}

> src/hotspot/share/opto/library_call.cpp line 4460:
> 
>> 4458:       if (init_out->is_CastII() && init_out->in(0) == init_control && init_out->in(1) == alloc_length) {
>> 4459: #ifdef ASSERT
>> 4460:         if (prev_cast == NULL) {
> 
> Add comment why multiply `CastII` could be here (because each load_array_length() call in guard checks will generate now separate CastII nodes). All these CastII nodes are replaced here with one: original `alloc_length`.
> 
> I am wondering if will affect result of guard checks in runtime.

done.

Would you share more detail about your concern for "if will affect result of guard checks in runtime."?

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

PR: https://git.openjdk.java.net/jdk17/pull/49