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

[Christian Hagedorn]

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. 

This assert has proven to be quite valuable to find problems in the memory graph that we would otherwise miss. It was also one of the few assert that triggered when having a corrupted graph due to missing Assertion Predicates. I'm wondering if we need more such memory graph checks in general. Anyway, that's just a thought for some future 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");

test/hotspot/jtreg/compiler/types/TestBadMemSliceWithInterfaces.java line 56:

> 54:         A a;
> 55:         if (flag) {
> 56:            a = b;

Indentation is off:
Suggestion:

            a = b;

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21303#pullrequestreview-2342747463
PR Review Comment: https://git.openjdk.org/jdk/pull/21303#discussion_r1784414779
PR Review Comment: https://git.openjdk.org/jdk/pull/21303#discussion_r1784400108