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

[Tobias Hartmann]

On Fri, 14 Jun 2024 07:09:55 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> #### 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

Nice analysis and great refactoring Christian!

src/hotspot/share/opto/parse2.cpp line 445:

> 443:   if (!array_type->is_flat() && !array_type->is_not_flat()) {
> 444:     // For array accesses, it can be useful to speculate on flatness such that we can fix the data layout. We only want
> 445:     // to do that when we know nothing about flatness since it requires a trap when profiling turns out to be wrong.

I think we should rephrase this because we don't speculate "on flatness". Maybe something like:
"For arrays that might be flat, speculate that the array has the exact type reported in the profile data such that we can rely on a fixed memory layout".

src/hotspot/share/opto/parse2.cpp line 453:

> 451:   }
> 452: 
> 453:   // Even though the type does not tell us whether we have an inline type or not, we can still check the profile data

Suggestion:

  // Even though the type does not tell us whether we have an inline type array or not, we can still check the profile data

?

src/hotspot/share/opto/parse2.cpp line 555:

> 553: 
> 554: // Speculate that the array is non-flat. We emit a trap when this turns out to be wrong. On the fast path, we add a
> 555: // CheckCastPP to use the non-flat type..

Suggestion:

// CheckCastPP to use the non-flat type.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestSpeculateArrayAccess.java line 70:

> 68:             //
> 69:             // Running with -XX:+StressGCM: We could execute the LoadN before entering the loop.
> 70:             // This crashes when iFld = -1 becauase we then access an out-of-bounds element.

Suggestion:

            // This crashes when iFld = -1 because we then access an out-of-bounds element.

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

Marked as reviewed by thartmann (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1127#pullrequestreview-2117723860
PR Review Comment: https://git.openjdk.org/valhalla/pull/1127#discussion_r1639427815
PR Review Comment: https://git.openjdk.org/valhalla/pull/1127#discussion_r1639432283
PR Review Comment: https://git.openjdk.org/valhalla/pull/1127#discussion_r1639417853
PR Review Comment: https://git.openjdk.org/valhalla/pull/1127#discussion_r1639414955