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

[Christian Hagedorn]

On Wed, 2 Oct 2024 13:11:18 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 th...
>
> 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 six additional commits since the last revision:
> 
>  - review
>  - Merge branch 'master' into JDK-8340214
>  - Update src/hotspot/share/opto/compile.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update test/hotspot/jtreg/compiler/types/TestBadMemSliceWithInterfaces.java
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - test cleanup
>  - fix & test

Still good, one more minor thing.

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

> 66:         A a;
> 67:         if (flag) {
> 68:            a = b;

Suggestion:

            a = b;

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21303#pullrequestreview-2343020437
PR Review Comment: https://git.openjdk.org/jdk/pull/21303#discussion_r1784567122