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

[Christian Hagedorn]

On Mon, 17 Jun 2024 07:19: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
>
> Christian Hagedorn has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Update comments
>  - Apply suggestions from code review
>    
>    Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>

Thanks Tobias for your review! As we've discussed offline, I've pushed an additional fix for a case that was missing: The load can still float above a range check when it's pinned at the If with a `FlatArrayCheck` which directly follows the range check. To avoid that, we pin it with `C2_UNKNOWN_CONTROL_LOAD` because we cannot set two nodes as control dependency.

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

PR Comment: https://git.openjdk.org/valhalla/pull/1127#issuecomment-2175578075