> This PR tightens the conditions under which an optimization from > `InlineTypeNode::Ideal` should be carried out. This optimization was > initially reported as missed with `-XX:VerifyIterativeGVN`. > > ### Summary > > The original failure appeared with `TestFieldNullMarkers.java` and > `-XX:VerifyIterativeGVN=1110`. This test performs various allocations with > value classes and other Valhalla features. > > In `InlineTypeNode::Ideal`, we have the following optimization: > > https://github.com/openjdk/valhalla/blob/b1d14c658511cced2a860d9e2741ae89827e25ba/src/hotspot/share/opto/inlinetypenode.cpp#L835-L853 > > As explained in the optimization comments, we don't want to use the base oop > if it corresponds to the larval oop. This is enforced by the condition > `AllocateNode::Ideal_allocation(base) == nullptr`. > > In our case this is exactly what happens, and the optimization is prevented. > HOwever, this changes during macro expansion. The allocate node disappears, > and `AllocateNode::Ideal_allocation(base) == nullptr` becomes true. As this > is not intended, there is no notification mechanism for this case and we end > up with the missing optimization assert. > > The conditions for this bug to reproduce are somewhat subtle. When things go > well, the larval `Allocate` node is eliminated by `eliminate_allocate_node` > before this problematic case shows up. However, there are cases where the > `<init>` method is not inlined, and this prevents the removal of the > `Allocate` node. It stays until macro expansion, and that is where things go > wrong. > > In the extracted reproducer, the `<init>` method is not inlined because of > unloaded signature classes, as `CompileCommand=printinlining` shows: > > > CompileCommand: PrintInlining *.* bool PrintInlining = true > @ 5 > compiler.valhalla.inlinetypes.TestMissingOptUseBaseOop$MyValue::<init> (10 > bytes) failed to inline: unloaded signature classes > > > This could also happen for other reasons though. > > ### Solution > > The solution is to not do this optimization after macro expansion. Adding a > `phase->C->allow_macro_nodes()` check ensures that the > `AllocateNode::Ideal_allocation(base)` call is relevant in the current phase > and that we can use it to check if we are dealing with the larval oop. > > ### Testing > - [x] [GitHub > Actions](https://github.com/benoitmaillard/valhalla/actions?query=branch%3AJDK-8367623) > - [x] tier1-4, plus some internal testing
Benoît Maillard has updated the pull request incrementally with one additional commit since the last revision: Add missing number Add bug reference for missed optimization in InlineType. ------------- Changes: - all: https://git.openjdk.org/valhalla/pull/1742/files - new: https://git.openjdk.org/valhalla/pull/1742/files/43b36743..77836405 Webrevs: - full: https://webrevs.openjdk.org/?repo=valhalla&pr=1742&range=01 - incr: https://webrevs.openjdk.org/?repo=valhalla&pr=1742&range=00-01 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/valhalla/pull/1742.diff Fetch: git fetch https://git.openjdk.org/valhalla.git pull/1742/head:pull/1742 PR: https://git.openjdk.org/valhalla/pull/1742
