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

[Roland Westrelin]

> 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.

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

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/21303/files
  - new: https://git.openjdk.org/jdk/pull/21303/files/913a82b4..46042b26

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=21303&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21303&range=01-02

  Stats: 3240 lines in 131 files changed: 2545 ins; 329 del; 366 mod
  Patch: https://git.openjdk.org/jdk/pull/21303.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/21303/head:pull/21303

PR: https://git.openjdk.org/jdk/pull/21303