[lworld] RFR: 8315003: [lworld] C2: Implement full support for JDK-8287061 and JDK-8316991

Christian Hagedorn chagedorn at openjdk.org
Wed Oct 9 12:41:52 UTC 2024


This patch makes the improved allocation merge at EA work with inline types.

Before this patch, the assumption was that we do **not** have non-scalarized inline types at safepoints which EA is going to scalarize (and if we do, then there is a missed opportunity which can be considered a bug). We should have pushed all `InlineTypeNodes` down through phis in order to have them scalarized before EA.

This, however, changes with the improved allocation merge RFEs that have been merged into Valhalla ([JDK-8287061](https://bugs.openjdk.org/browse/JDK-8287061) and [JDK-8316991](https://bugs.openjdk.org/browse/JDK-8316991)). We could now the following graph as found in the test case:

![image](https://github.com/user-attachments/assets/4e5174d3-9a52-4c43-8dcd-b6a119438ca0)

Here, `272 Phi` merges the non-value type allocation `238 CheckCastPP` with the value type allocation `217 InlineType`. Before the improved allocation merge RFEs, we would not be able to remove the allocations. But now, we can do it at EA: We create a special `SafePointScalarMergeNode` which in case has `SafePointScalarObjectNodes` as inputs for the merged allocations. Additionally, each field of the merged allocations are added to the safepoint, which also includes the value type field `v2` of `V`:

![image](https://github.com/user-attachments/assets/eb8e7012-262d-492f-ae30-a3962f5ae931)

We should not have safepoint uses for `InlineType` and thus now replace them with `SafePointScalarObjectNodes` as well (newly done with this patch). As a result, we have the following graph after EA:

![image](https://github.com/user-attachments/assets/0cf521c4-dee1-4cb7-bb7c-3479b459ed34)

Note that the safepoint node inputs for fields are well-defined to keep track of which field belong to each of the `SafePointScalarObjectNodes` of the `SafePointScalarMergeNode`.


### Additional Code Changes
- There was a bug in mainline with the new allocation merge implementation that is already fixed in mainline but not integrated in Valhalla, yet: [JDK-8335220](https://bugs.openjdk.org/browse/JDK-8335220). I cherry-picked these changes (L589-605 in escape.cpp).
- I added assertion code to verify that when we are going to scalar replace an inline type allocation that is an input into a phi at EA, we would not have been able to push the `InlineTypeNode` down (which should have been done earlier if possible).
- Re-enabled uncommented tests and removed problem-listed `AllocationMergesTests.java` that had been added with the allocation merge RFE.

### Future Work
There is currently an open bug for the allocation merges ([JDK-8335977](https://bugs.openjdk.org/browse/JDK-8335977)) which has been found by extending `AllocationMergesTests.java` with various deoptimization points and checking for correct values (similar to what we do in `TestValueConstruction.java`). Once this bug is fixed in mainline (it should extend this test and add these deoptimization checks), we should extend `AllocationMergesTests.java` and add tests for value types as well. I filed [JDK-8341856](https://bugs.openjdk.org/browse/JDK-8341856) to keep track of that.

Thanks,
Christian

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

Commit messages:
 - 8315003: [lworld] C2: Implement full support for JDK-8287061 and JDK-8316991

Changes: https://git.openjdk.org/valhalla/pull/1271/files
  Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=1271&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8315003
  Stats: 155 lines in 7 files changed: 134 ins; 11 del; 10 mod
  Patch: https://git.openjdk.org/valhalla/pull/1271.diff
  Fetch: git fetch https://git.openjdk.org/valhalla.git pull/1271/head:pull/1271

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


More information about the valhalla-dev mailing list