When slightly modifying the test that I added for 
[JDK-8386067](https://bugs.openjdk.org/browse/JDK-8386067), I hit an assert in 
`CallStaticJavaNode::replace_is_substitutable` because expansion of the 
substitutability call failed and `emit_substitutability_check` returned 
`nullptr` although `can_emit_substitutability_check` returned `true`.

In the new test, the problematic field is `ObjectHolder::obj` which, although 
it is of type `Object`, is always `exact` and therefore can't be a value object 
and does not require a substitutability test. 
`InlineTypeNode::can_emit_substitutability_check` correctly returns `true`:
https://github.com/openjdk/valhalla/blob/2ae8e47da016bb4d7718e7923c964c2b91c638e6/src/hotspot/share/opto/inlinetypenode.cpp#L611-L614


However, during recursive expansion, nullable value object fields are 
null-checked and then compared field-by-field:
https://github.com/openjdk/valhalla/blob/2ae8e47da016bb4d7718e7923c964c2b91c638e6/src/hotspot/share/opto/inlinetypenode.cpp#L910

The code unconditionally rebuilt those fields with 
`InlineTypeNode::make_from_oop` after the null and type checks because there's 
a `CastPP` in-between:
https://github.com/openjdk/valhalla/blob/2ae8e47da016bb4d7718e7923c964c2b91c638e6/src/hotspot/share/opto/inlinetypenode.cpp#L955-L960

 If the field was already scalarized, this discarded the existing 
`InlineTypeNode` and reloaded fields from the oop, losing precise information 
such as exact Object field Phis. The later Object field comparison then looks 
non-exact and `emit_substitutability_check_pointer` returns `nullptr`:
https://github.com/openjdk/valhalla/blob/2ae8e47da016bb4d7718e7923c964c2b91c638e6/src/hotspot/share/opto/inlinetypenode.cpp#L722-L724

The fix is to preserve already-scalarized recursive field operands when their 
inline klass matches the comparison klass, and only checkcast/reload operands 
that are not scalarized. With the fix, exact type information is preserved and 
we emit a pointer comparison for the exact object fields:
https://github.com/openjdk/valhalla/blob/2ae8e47da016bb4d7718e7923c964c2b91c638e6/src/hotspot/share/opto/inlinetypenode.cpp#L700-L702

The IR Framework test verifies this successful expansion.

Note: The null and type checks are not immediately folded because by GVN 
because we need to set `set_delay_transform` here:
https://github.com/openjdk/valhalla/blob/2ae8e47da016bb4d7718e7923c964c2b91c638e6/src/hotspot/share/opto/callnode.cpp#L1412-L1416

Thanks,
Tobias

---------
- [x] I confirm that I make this contribution in accordance with the [OpenJDK 
Interim AI Policy](https://openjdk.org/legal/ai).

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

Commit messages:
 - Found another missing TODO
 - Workaround for C1 issue
 - Prototype

Changes: https://git.openjdk.org/valhalla/pull/2602/files
  Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=2602&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8387488
  Stats: 57 lines in 3 files changed: 45 ins; 2 del; 10 mod
  Patch: https://git.openjdk.org/valhalla/pull/2602.diff
  Fetch: git fetch https://git.openjdk.org/valhalla.git pull/2602/head:pull/2602

PR: https://git.openjdk.org/valhalla/pull/2602

Reply via email to