## Problem
When C2 parses a variable, it tries to be smart about the type of that
variable. If a method parameter declares that it is of type `A`, and `A` is an
abstract class that only has a single concrete subclass `B`, then C2 eagerly
assumes that the parameter must be of type `B`. This property, in the presence
of concurrent class loading/unloading, does not always hold, as the VM can
always load another concrete subclass `C` of `A` that is unrelated to `B`. To
avoid incorrect execution arised from this issue, C2 relies on `dependencies`,
which encodes various kinds of assumptions the compiler made. Among those, we
have an assumption called `unique_concrete_subclass`, which will be invalidated
when `C` is loaded, triggering the deoptimization and invalidation of the
aforementioned compiled method.
However, this mechanism is both inadequate and not timely. The main issue is
that the concurrent load of `C` may not only happen after the compilation has
finished, but also during the compilation process. This leads to CHA
computation giving contradictory results when called at different time in the
compilation process. There are 2 examples which were reported:
1.
if (b) {
A a1 = ...;
} else {
C a2 = ...;
}
A a = Phi(a1, a2);
If `A a = Phi(a1, a2)` is parsed before `C a2 = ...`, and `C` has not been
loaded then, the compiler will assume that `a` must be of type `B`. This leads
to a `Phi` of type `B` with one of its inputs being a node of type `C` which is
unrelated. This graph shape is nonsense, and all kinds of issues can appear
from various optimization passes which do not expect such a conflicting graph.
2.
A a = call();
If when `a` is parsed, `C` has not been loaded, the compiler will assume `a` is
of type `B`. However, if `call` is late-inlined, and it turns out that the
returned value is of type `C`. All uses of `a` will suddenly have their input
changed from a `B` to a `C`, which may also lead to unexpected failures.
## Overview of this PR
This PR tries to detect CHA inconsistencies, and triggers a compilation failure
when such a paradox happens. The core invariant that needs holding is that,
given type `X` and `Y`, if `X` is a subtype of `Y`, then the result of
narrowing `X` should also be a subtype of the result of narrowing `Y`. This
invariant does not need to hold in the VM, it only needs to hold in the local
`ciEnv`, which is the local view of the compiler with respect to the class
hierarchy.
In some cases, we can avoid such a paradox from arising, such as when CHA would
load a class that would lead to a paradoxical hierarchy view. In those cases,
we can do a pre-check and avoid loading that class into the `ciEnv`, so that
the compiler is not exposed to that paradox. In other cases, loading the class
into the `ciEnv` is inevitable, as it appears in the class file. In those
cases, the `ciEnv` instance will record the paradox, and C2 will abort
compilation accordingly.
## Design considerations
#### Avoid failing the compilation
An alternative approach I have pursued is to ensure that a paradoxical
hierarchy view is always avoided. This has a couple of drawbacks:
- Since parsing can lead to several classes that must be loaded. We either need
to change it so that parsing will stop if it encounters a class that leads to a
paradox, or move all CHA inference to after all parsing, or even after escape
analysis. Both approaches are complicated, and it is hard to ensure that a
class is never loaded so that a paradox is seen because there are many places
which load them. One of the unexpected places that loads `ciInstanceKlass` is
`ciMethod::method_data` which may load classes corresponding to the profiled
receiver.
- There are a lot of optimizations that benefit from a class having a sharp
type. In principle, they should also be applicable during IGVN, but doing so
would require non-trivial effort.
#### Paradoxical hierarchy immediately fails the `ciEnv`
Currently, when a paradoxical hierarchy is found, we mark the corresponding
flag in `ciEnv` without failing it. This is because only C2 cares about the
consistency of CHA computation, `ciTypeFlow` can work just fine. In addition,
`ciTypeFlow` does not expect spontaneous failures when calling
`ciEnv::get_instance_klass`, so it would be cleaner to make only C2 check for
such inconsistencies.
## Testing
I have not managed to come up with a dedicated test case for this fix, because
there are only a limited number of cases where the inconsistency leads to
incorrectly compiled code and the compiled code is not discarded by
dependencies. As a result, I reverted the related changes that were temporary
fixes for this issue and observed no failure from the tests added by those
fixes.
- [ ] tier1-4,hs-comp-stress,hs-aot-stress
## Some adjustments to existing code and tests
- `ciKlass::is_leaf_type` is unused.
- `Dependency::check_unique_implementor` seems dubious, there is no guarantee
that the assumption is not violated after the call to
`ciInstanceKlass::unique_implementor` and before the call to
`Dependencies::assert_unique_implementor`.
- `TestMergeStore` assumes that `Cloneable` is always ordered before
`Serializeable` when printing an array klass, this is not true, the added
interfaces also do not add any more verification rigor, so I removed them.
- In `TestNarrowPhi`, I replaced the combination of calling
`WhiteBox::enqueueMethodForCompilation` and waiting for
`WhiteBox::isMethodQueuedForCompilation` with simply waiting for
`WhiteBox::enqueueMethodForCompilation` under `-Xbatch`, which achieves the
same effect but is much better.
- In `Continuation/Fuzz.java`, the dance of calling
`enqueueMethodForCompilation` and `isMethodQueuedForCompilation` does not work
when the compilation may fail. Especially for complicated methods such as those
in the tests. I also replace it with simply calling
`enqueueMethodForCompilation` under `-Xbatch`.
Please take a look and leave your reviews, thanks a lot.
---------
- [x] I confirm that I make this contribution in accordance with the [OpenJDK
Interim AI Policy](https://openjdk.org/legal/ai).
-------------
Commit messages:
- Fix Coninuation/Fuzz, revert PhaseCCP temporary fix
- Fix keep_alive mechanism, remove old Phi input workaround, fix test
failures, change paradoxical_hierarchy not to fail ciEnv
- has_subklass may be called from VM
- Fix keep_alive
- Ensure the hierarchy view is consistent
Changes: https://git.openjdk.org/jdk/pull/33753/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=33753&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8388184
Stats: 1006 lines in 20 files changed: 459 ins; 260 del; 287 mod
Patch: https://git.openjdk.org/jdk/pull/33753.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/33753/head:pull/33753
PR: https://git.openjdk.org/jdk/pull/33753
