RFR: 8323274: C2: array load may float above range check [v3]

[Emanuel Peter]

On Wed, 7 Feb 2024 13:17:32 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> This PR includes 5 test cases in which an array load floats above its
>> range check and the resulting compiled code can be made to segfault by
>> passing an out of bound index to the test method. Each test case takes
>> advantage of a different transformation to make the array load happen
>> too early:
>> 
>> For instance, with TestArrayAccessAboveRCAfterSplitIf:
>> 
>> 
>> if (k == 2) {
>>     v = array1[i];
>>     array = array1;
>>     if (l == m) {
>>     }
>> } else {
>>     v = array2[i];
>>     array = array2;
>> }
>> v += array[i]; // range check + array load
>> 
>> 
>> The range check is split through phi:
>> 
>> 
>> if (k == 2) {
>>     v = array1[i];
>>     array = array1;
>>     if (l == m) {
>>     }
>>     // range check here
>> } else {
>>     v = array2[i];
>>     array = array2;
>>     // range check here
>> }
>> v += array[i]; // array load
>> 
>> 
>> Then an identical dominating range check is found:
>> 
>> 
>> if (k == 2) {
>>     v = array1[i]; // range check here
>>     array = array1;
>>     if (l == m) {
>>     }
>> } else {
>>     v = array2[i];  // range check here
>>     array = array2;
>> }
>> v += array[i]; // array load
>> 
>> 
>> Then a branch dies:
>> 
>> 
>> v = array1[i]; // range check here
>> array = array1;
>> if (l == m) {
>> }
>> v += array[i]; // array load
>> 
>> 
>> The array load is dependent on the `if (l == m) {` condition. An
>> identical dominating condition is then found which causes the control
>> dependent range check to float above the range check.
>> 
>> Something similar can be triggered with:
>> 
>> - TestArrayAccessAboveRCAfterPartialPeeling: sometimes, during partial
>>   peeling a load is assigned the loop head as control so something
>>   gets in between the range check and an array load and steps similar
>>   to the above can cause the array load to float above its range check.
>>   
>> - TestArrayAccessAboveRCAfterUnswitching: cloning a loop body adds
>>   regions on exits of the loop and nodes that only have uses out of
>>   the loop can end up control dependent on one of the regions. In the
>>   test case, unswitching is what causes the cloning to happen. Again
>>   similar steps as above make the array load floats above its range
>>   check. I suppose similar bugs could be triggered with other loop
>>   transformations that rely on loop body cloning.
>>   
>> TestArrayAccessAboveRCAfterSinking is a bit different in that it can
>> change the control of an array load to be the projection of some
>> arbitrary test. That test can then be replaced by a dominating one
>> causing t...
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update src/hotspot/share/opto/loopopts.cpp
>   
>   Co-authored-by: Emanuel Peter <emanuel.peter at oracle.com>

@rwestrel just ping me again when I should re-review ;)

src/hotspot/share/opto/loopnode.hpp line 1743:

> 1741:   bool can_move_to_inner_loop(Node* n, LoopNode* n_loop, Node* x);
> 1742: 
> 1743:   void pin_array_access_nodes(Node* ctrl);

Suggestion:

  void pin_array_access_nodes_dependent_on(Node* ctrl);

Does this even build without?

src/hotspot/share/opto/loopopts.cpp line 1496:

> 1494:           // 1. Move from RangeCheck "a" to RangeCheck "b": don't need to pin. If we ever remove b, then we pin all its array accesses at that point.
> 1495:           // 2. We move from RangeCheck "a" to regular if "b": need to pin. If we ever remove b, then its array accesses would start to float, since we don't pin at that point.
> 1496:           // 3. If we move from regular if: don't pin. All array accesses are already assumed to be pinned.

Suggestion:

          // Split if: pin array accesses that are control dependent on a range check and moved to a regular if,
          // to prevent an array load from floating above its range check. There are three cases:
          // 1. Move from RangeCheck "a" to RangeCheck "b": don't need to pin. If we ever remove b, then we pin
          //    all its array accesses at that point.
          // 2. We move from RangeCheck "a" to regular if "b": need to pin. If we ever remove b, then its array
          //    accesses would start to float, since we don't pin at that point.
          // 3. If we move from regular if: don't pin. All array accesses are already assumed to be pinned.

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

PR Comment: https://git.openjdk.org/jdk/pull/17635#issuecomment-1932039801
PR Review Comment: https://git.openjdk.org/jdk/pull/17635#discussion_r1481464265
PR Review Comment: https://git.openjdk.org/jdk/pull/17635#discussion_r1481465672