RFR: 8340214: C2 compilation asserts with "no node with a side effect" in PhaseIdealLoop::try_sink_out_of_loop

[Tobias Hartmann]

On Wed, 2 Oct 2024 11:21:43 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> The patch includes 2 test cases for this: test1() causes the assert
> failure in the bug description, test2() causes an incorrect execution
> where a load floats above a store that it should be dependent on.
> 
> In the test cases, `field` is accessed on object `a` of type `A`. When
> the field is accessed, the type that c2 has for `a` is `A` with
> interface `I`. The holder of the field is class `A` which implements
> no interface. The reason the type of `a` and the type of the holder
> are slightly different is because `a` is the result of a merge of
> objects of subclasses `B` and `C` which implements `I`.
> 
> The root cause of the bug is that `Compile::flatten_alias_type()`
> doesn't change `A` + interface `I` into `A`, the actual holder of the
> field. So `field` in `A` + interface `I` and `field` in `A` get
> different slices which is wrong. At parse time, the logic that creates
> the `Store` node uses:
> 
> 
> C->alias_type(field)->adr_type()
> 
> 
> to compute the slice which is the slice for `field` in `A`. So the
> slice used at parse time is the right one but during igvn, when the
> slice is computed from the input address, a different slice (the one
> for `A` + interface `I`) is used. That causes load/store nodes when
> they are processed by igvn to use the wrong memory state.
> 
> In `Compile::flatten_alias_type()`:
> 
> 
> if (!ik->equals(canonical_holder) || tj->offset() != offset) {
>   if( is_known_inst ) {
>     tj = to = TypeInstPtr::make(to->ptr(), canonical_holder, true, nullptr, offset, to->instance_id());
>   } else {
>     tj = to = TypeInstPtr::make(to->ptr(), canonical_holder, false, nullptr, offset);
>   }
> }
> 
> 
> only flattens the type if it's not the canonical holder but it should
> test that the type doesn't implement interfaces that the canonical
> holder doesn't. To keep the logic simple, the fix I propose creates a
> new type whenever there's a chance that a type implements extra
> interfaces (the type is not exact).
> 
> I also added asserts in `GraphKit::make_load()` and
> `GraphKit::store_to_memory()` to make sure the slice that is passed
> and the address type agree. Those asserts fire with the new test
> cases. When running testing, I found that they also catch a few cases
> in `library_call.cpp` where an incorrect slice is passed.
> 
> As further clean up, maybe we want to drop the slice argument to
> `GraphKit::make_load()` and `GraphKit::store_to_memory()` (and to
> their callers) given it's redundant with the address type and error
> prone.

Looks reasonable to me. I submitted testing and will report back once it passed. 

> As further clean up, maybe we want to drop the slice argument to GraphKit::make_load() and GraphKit::store_to_memory() (and to their callers) given it's redundant with the address type and error prone.

Yes, let's do that. Please file a starter RFE.

src/hotspot/share/opto/compile.cpp line 1468:

> 1466:       ciInstanceKlass *canonical_holder = ik->get_canonical_holder(offset);
> 1467:       assert(offset < canonical_holder->layout_helper_size_in_bytes(), "");
> 1468:       assert(tj->offset() == offset, "not change to offset expected");

Suggestion:

      assert(tj->offset() == offset, "no change to offset expected");

src/hotspot/share/opto/compile.cpp line 1475:

> 1473:         assert(tj == TypeInstPtr::make(to->ptr(), canonical_holder, is_known_inst, nullptr, offset, instance_id), "exact type should be canonical type");
> 1474:       } else {
> 1475:         assert(xk || !is_known_inst, "Known instance should be exact type");

Maybe add a comment here and explain the two cases when we create a new type.

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21303#pullrequestreview-2342672361
PR Review Comment: https://git.openjdk.org/jdk/pull/21303#discussion_r1784341679
PR Review Comment: https://git.openjdk.org/jdk/pull/21303#discussion_r1784422309