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)
- [ ] tier1-4, plus some internal testing

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

Commit messages:
 - Add comment to explain the inlining failure
 - Rename test, add missing stuff
 - Implement actual fix
 - Update new test
 - Add new test

Changes: https://git.openjdk.org/valhalla/pull/1742/files
  Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=1742&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8367623
  Stats: 70 lines in 2 files changed: 69 ins; 0 del; 1 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

Reply via email to