[lworld] RFR: 8333889: [lworld] C2: Hoisting speculative array access type check wrongly moves array access before its range check

Christian Hagedorn chagedorn at openjdk.org
Fri Jun 14 07:17:39 UTC 2024


#### Speculative Inline Type Checks for `aaload`

In `Parse::array_addressing()`, we emit speculative inline type checks (about non-flatness and non-null-freeness) with traps based on profiling information. These checks are emitted directly after the `RangeCheck`. This will lead to the situation that the `CastII` and actual `LoadN` nodes are disconnected from the `RangeCheck` true projection:

![image](https://github.com/openjdk/valhalla/assets/17833009/b82e4b5c-20b5-4c0f-8099-57883aae19d1)

#### Known Problem when Load is Disconnected from Range Check

This disconnection of the load from its range check is a known general problem and could lead to the situation where the actual load ends up before the range check due to some optimizations. As a result, we could crash due to an unprotected out-of-bounds access when the load is actually scheduled before the range check with an invalid index. This was observed before on several occasions in mainline and was being fixed there (e.g. [JDK-8323274](https://bugs.openjdk.org/browse/JDK-8323274)). 

#### How this Problem Manifests in Valhalla

In the test case, the very same is happening:

1. Loop Predication hoists `237 If` out of the loop since it's invariant.
2. `108 RangeCheck` cannot be hoisted since it's not invariant and we have a non-counted loop with a `float` iv phi.
3. By hoisting `237 If` out of the loop, we also rewire the data dependencies (i.e. `140 CastII` and `146 LoadN`) out of the loop. These now end up before the range check which is still inside the loop.
4. When running with `-XX:+StressGCM`, the `LoadN` could be scheduled before the actual `RangeCheck` inside the loop and we crash when using an invalid out-of-bounds array index.

 #### Solution: Swap `RangeCheck` and Speculative Inline Type Traps
The fix is straight forward to first emit the speculative inline type checks with its traps and only then emit the `RangeCheck` such that it does not lose the connection to the actual load. 

I've additionally done some refactoring and added some comments which helped me to better understand the code.

Thanks,
Christian

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

Commit messages:
 - 8333889: [lworld] C2: Hoisting speculative array access type check wrongly moves array access before its range check

Changes: https://git.openjdk.org/valhalla/pull/1127/files
  Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=1127&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8333889
  Stats: 345 lines in 3 files changed: 195 ins; 76 del; 74 mod
  Patch: https://git.openjdk.org/valhalla/pull/1127.diff
  Fetch: git fetch https://git.openjdk.org/valhalla.git pull/1127/head:pull/1127

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



More information about the valhalla-dev mailing list