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

[Vladimir Kozlov]

On Tue, 15 Jun 2021 02:08:30 GMT, Hui Shi <hshi at openjdk.org> wrote:

> This redos PR https://github.com/openjdk/jdk/pull/4238
> 
> Some closed tests fail due to original PR, the main reasons are:
> 
> 8268325/8268345
> CastIINode created in AllocateArrayNode::make_ideal_length apply on a negative array length. GVN transform converts these CastIINode to top. This breaks assumptions, eg., array index node type is TypeInt.
> 8268345
> Some CastIINode is created in LoadRange::Ideal, which doesn't apply GVN transform. CastIINode can not be found in GVN hashtable and cause assertion failure.
> Based on previous PR #4238, this PR unifies CastIINode processing post AllocateArrayNode::make_ideal_length. Setup type and add into igvn worklist. Avoid apply GVN transform to break different assumptions in parser.
> 
> Based on closed test failrue message, two new tests are added
> 
> TestNegArrayLengthAsIndex1 : fail in debug build even without previous PR #4238, it loads a negative array length and apply GVN transform (In GraphKit::load_array_length) which covert length to top node. It also assert in Parse::array_addressing.
> 
> TestNegArrayLengthAsIndex2 : similar with failrue in https://bugs.openjdk.java.net/browse/JDK-8268301

Few 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()`.

src/hotspot/share/opto/library_call.cpp line 4457:

> 4455: #endif
> 4456:     for (uint i = 0; i < init_control->outcnt(); i++) {
> 4457:       Node *init_out = init_control->raw_out(i);

Style: `Node* init_out`

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.

src/hotspot/share/opto/library_call.cpp line 4513:

> 4511:       if (ccast != length) {
> 4512:         _gvn.set_type_bottom(ccast);
> 4513:         record_for_igvn(ccast);

Use wrapper method to avoid duplication.

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

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