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

[Christian Hagedorn]

On Wed, 2 Oct 2024 13:58:14 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 incrementally with one additional commit since the last revision:
> 
>   Update test/hotspot/jtreg/compiler/types/TestBadMemSliceWithInterfaces.java
>   
>   Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>

Marked as reviewed by chagedorn (Reviewer).

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

PR Review: https://git.openjdk.org/jdk/pull/21303#pullrequestreview-2344788165