RFR: 8292301: [REDO v2] C2 crash when allocating array of size too large [v2]

[Tobias Hartmann]

On Fri, 2 Sep 2022 09:02:54 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> On top of the redo, this fixed 2 bugs:
>> 
>> 8288184: the problem here is that the ValidLengthTest input of an
>> AllocateArrayNode becomes a constant. The CatchNode would then change
>> types if it was reprocessed but it's not. Custom logic is needed to
>> enqueue the CatchNode when the ValidLengthTest input of an
>> AllocateArrayNode changes. The CastII out of the AllocateArrayNode
>> becomes top but the fallthrough path doesn't die. This happens with
>> igvn in the case of the bug but could also happen with ccp. I fixed
>> both in this patch.
>> 
>> 8291665: the code pattern for this is 2 AllocateArrayNodes out of loop
>> with a shared ValidLengthTest input in a loop. When the loop is cloned
>> that causes Phis to be added between the AllocateArrayNodes and the
>> BoolNode of the ValidLengthTest inputs. Split if runs next and it
>> doesn't expect the Phi at the ValidLengthTest inputs. The fix here is
>> to clone the Bool/Cmp subgraph down on loop cloning. There's logic for
>> that when the use of the bool is an If for instance so I simply added
>> a special case to run that logic for an AllocateArrayNode use as
>> well. Note that the test case I added fails reliably on 11 but not
>> with the current jdk developement branch. AFAICT, the bug is there but
>> something unrelated changed and a slightly different graph is built
>> for the test case that prevents split if.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   undo needless change

Looks good otherwise.

src/hotspot/share/opto/loopopts.cpp line 2040:

> 2038:       // loop to determine which way the loop exited.
> 2039:       // Loop predicate If node connects to Bool node through Opaque1 node.
> 2040:       if (use->is_If() || use->is_CMove() || C->is_predicate_opaq(use) || use->Opcode() == Op_Opaque4 ||

Please add a comment describing the new case.

src/hotspot/share/opto/loopopts.cpp line 2410:

> 2408:     while (split_if_set->size()) {
> 2409:       Node *iff = split_if_set->pop();
> 2410:       uint input = iff->Opcode() == Op_AllocateArray ? AllocateNode::ValidLengthTest : 1;

Suggestion:

      uint input = (iff->Opcode() == Op_AllocateArray) ? AllocateNode::ValidLengthTest : 1;

src/hotspot/share/opto/phaseX.cpp line 1643:

> 1641:       }
> 1642:     }
> 1643:     if (use_op == Op_AllocateArray && n == use->in(AllocateNode::ValidLengthTest)) {

Please add a comment.

src/hotspot/share/opto/phaseX.cpp line 1853:

> 1851: // If we changed the receiver type to a call, we need to revisit the Catch node following the call. It's looking for a
> 1852: // non-NULL receiver to know when to enable the regular fall-through path in addition to the NullPtrException path.
> 1853: // Same if true if the type of a ValidLengthTest input to an AllocateArrayNode changes

Suggestion:

// Same is true if the type of a ValidLengthTest input to an AllocateArrayNode changes.

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

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10038