[lworld] RFR: 8293541: [lworld] IR verification fails for TestLWorld::test109_sharp and test110_sharp
Christian Hagedorn
chagedorn at openjdk.org
Thu Mar 7 07:40:57 UTC 2024
`test109_sharp()` and `test110_sharp()` originally failed because we have no longer beeen able to apply Loop Predication due to the following merge mistake (left previous, middle merged, right new code from mainline):

This changed the application of EA for these tests in such a way that we need to apply EA twice (iteratively) to remove the wrapping box object and the inline type allocation. This had the side effect that we missed to re-add a memory phi to the IGVN worklist to apply an optimization to get rid of a diamond-if control flow which blocks Loop Predication. IR matching failed because we could not find the corresponding predicate trap for an expected Hoisted Check Predicate.
This was also a problem in mainline but we had no IR test to catch this. [JDK-8314997](https://bugs.openjdk.org/browse/JDK-8314997) fixed this for mainline and once it was merged with Valhalla, `test109_sharp()` and `test110_sharp()` would now work again.
But I think we should still fix the merge mistake to eagerly try to remove inline type allocations, regardless of their escape/replaceable status as it was originally intended before the merge. This allows us to get rid of some extra EA iterations.
However, fixing this merge mistake caused an assertion failure later in EA which checked that safepoints do not have inline type allocation uses (they should be replaced by `SafePointScalarObjectNodes`). To fix that, we need to make sure that any inline type pointer phi node created for an inline type field here (`create_scalarized_object_description()` -> `value_from_mem()` -> `value_from_mem_phi()`):
https://github.com/openjdk/valhalla/blob/814a5e93cb392a77a43550bf8dab5ab9d52bdcfd/src/hotspot/share/opto/macro.cpp#L378-L379
when trying to create a `SafePointScalarObjectNode` for a safepoint use is eagerly replaced by an `InlineTypeNode`. To achieve that, I've added a call to `push_inline_types_through()` (renamed to `push_inline_types_down()`) which was only used in `PhiNode::Ideal()` before. I've applied some refactoring and renaming to share that code with EA
I've tried out some additional assertions in EA that an inline type pointer phi should already be replaced by an `InlineTypeNode` if possible. But that failed. I think we are missing some optimization opportunities here. As this might be out of scope of this bug, I suggest to file a separate RFE to investigate further.
#### Disabled Test Due to Existing Bug
I disabled TestOnStackReplacement::test3() by uncommenting it. It triggered the following assertion:
assert(outer->outcnt() >= phis + 2 - be_loads && outer->outcnt() <= phis + 2 + stores + 1) failed: only phis
I've had a closer look and it seems like this patch here just revealed an existing rare issue that was only seen with this one test and some stress options. I've filed [JDK-8327465](https://bugs.openjdk.org/browse/JDK-8327465) to not block this fix and follow up on it later.
#### Testing
I performed testing on an older repo state before transitioning to the JEP 401 model to verify the correctness of this patch.
Thanks,
Christian
-------------
Commit messages:
- 8293541: [lworld] IR verification fails for TestLWorld::test109_sharp and test110_sharp
Changes: https://git.openjdk.org/valhalla/pull/1038/files
Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=1038&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8293541
Stats: 190 lines in 5 files changed: 107 ins; 64 del; 19 mod
Patch: https://git.openjdk.org/valhalla/pull/1038.diff
Fetch: git fetch https://git.openjdk.org/valhalla.git pull/1038/head:pull/1038
PR: https://git.openjdk.org/valhalla/pull/1038
More information about the valhalla-dev
mailing list