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

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

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

Commit messages:
 - test cleanup
 - fix & test

Changes: https://git.openjdk.org/jdk/pull/21303/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21303&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8340214
  Stats: 121 lines in 4 files changed: 109 ins; 1 del; 11 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