RFR: 8346184: C2: assert(has_node(i)) failed during split thru phi [v2]

[Christian Hagedorn]

On Wed, 8 Jan 2025 13:42:18 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The assert fires during split thru phi because a call to `Identity`
>> returns a new node (a constant null pointer). That happens because a
>> `Load`, once pushed thru phi, can be constant folded because it loads
>> from a newly allocated array. `Identity` shouldn't return new
>> nodes. When split thru phi runs, in this case, `Value` should be the
>> one returning constant null, not `Identity`. There is logic for that
>> in `LoadNode::Value` but it's after some other checks that cause
>> `Value` to return too early.
>> 
>> To fix this, I propose reordering checks in `LoadNode::Value`.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - more
>  - Merge branch 'master' into JDK-8346184
>  - more
>  - test
>  - fix

The updated fix looks good. Let me run some testing again.

>> Should we generally add some verification code that Identity() does not create new nodes? Hard to do it at all places but it could, for example, be done in the transform() methods of GVN and IGVN when calling Identity(). But of course, should probably be done in a separate RFE.
>
> That sounds like a good idea. I filed: https://bugs.openjdk.org/browse/JDK-8347266

Thanks!

src/hotspot/share/opto/memnode.cpp line 2020:

> 2018:   // if the load is provably beyond the header of the object.
> 2019:   // (Also allow a variable load from a fresh array to produce zero.)
> 2020:   const TypeOopPtr *tinst = tp->isa_oopptr();

While at it:
Suggestion:

  const TypeOopPtr* tinst = tp->isa_oopptr();

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22818#pullrequestreview-2539669516
PR Review Comment: https://git.openjdk.org/jdk/pull/22818#discussion_r1908613661