RFR: 8323274: C2: array load may float above range check
Emanuel Peter
epeter at openjdk.org
Wed Feb 7 09:07:55 UTC 2024
On Tue, 30 Jan 2024 16:50:08 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 the array to float.
>
> Finally, in TestArrayAccessAboveRCForArrayCopyLoad, an array copy is
> converted to a series of loads/stores that's guarded by a test for
> `srcPos < dstPos...
Generally looks reasonable, I'm mostly suggesting cosmetic changes.
src/hotspot/share/opto/arraycopynode.cpp line 157:
> 155: // Pin the load: if this is an array load, it's going to be dependent on a condition that's not a range check for that
> 156: // access. If that condition is replaced by an identical dominating one, then an unpinned load would risk floating
> 157: // above runtime checks that guarantee it is withing bounds.
Suggestion:
// above runtime checks that guarantee it is within bounds.
src/hotspot/share/opto/loopopts.cpp line 1496:
> 1494: // Pin the array access nodes here to make sure no array access is then missed at split if.
> 1495: dominated_by(prevdom->as_IfProj(), n->as_If(), false,
> 1496: n->Opcode() == Op_RangeCheck && prevdom->in(0)->Opcode() != Op_RangeCheck);
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.
bool pin_array_access_nodes = n->Opcode() == Op_RangeCheck &&
prevdom->in(0)->Opcode() != Op_RangeCheck
dominated_by(prevdom->as_IfProj(), n->as_If(), false, pin_array_access_nodes);
"If a range check is replaced by a regular if, then that logic won't trigger."
Sounds like you don't want to pin if we move it to a regular if.
"Pin the array access nodes here to make sure no array access is then missed at split if."
Missed at split if: for what?
For my case 3: could we verify that the relevant array access nodes are already pinned?
src/hotspot/share/opto/loopopts.cpp line 1682:
> 1680: }
> 1681: }
> 1682: _igvn.replace_input_of(x, 0, c);
Suggestion:
Node* maybe_pinned_n = n;
Node* outside_ctrl = place_outside_loop(n_ctrl, loop_ctrl);
if (n->depends_only_on_test()) {
Node* pinned_clone = n->pin_array_access_node();
if (pinned_clone != nullptr) {
// Pin array access nodes: if this is an array load, it's going to be dependent on a condition that's not a
// range check for that access. If that condition is replaced by an identical dominating one, then an
// unpinned load would risk floating above its range check.
register_new_node(pinned_clone, outside_ctrl);
maybe_pinned_n = pinned_clone;
}
}
_igvn.replace_input_of(maybe_pinned_n, 0, outside_ctrl);
src/hotspot/share/opto/loopopts.cpp line 2265:
> 2263: clone->set_req(0, phi);
> 2264: register_new_node(clone, get_ctrl(use));
> 2265: _igvn.replace_node( use, clone);
Suggestion:
Node* pinned_clone = use->pin_array_access_node();
if (pinned_clone != nullptr) {
// Pin array access nodes: control is updated here to a region. If, after some transformations, only one path
// into the region is left, an array load could become dependent on a condition that's not a range check for
// that access. If that condition is replaced by an identical dominating one, then an unpinned load would risk
// floating above its range check.
pinned_clone->set_req(0, phi);
register_new_node(pinned_clone, get_ctrl(use));
_igvn.replace_node(use, pinned_clone);
src/hotspot/share/opto/split_if.cpp line 736:
> 734: // floating above its range check.
> 735: pin_array_access_nodes(new_true);
> 736: pin_array_access_nodes(new_false);
Suggestion:
pin_array_access_nodes_dependent_on(new_true);
pin_array_access_nodes_dependent_on(new_false);
src/hotspot/share/opto/split_if.cpp line 758:
> 756: if (clone != nullptr) {
> 757: register_new_node(clone, get_ctrl(u));
> 758: _igvn.replace_node(u, clone);
Suggestion:
void PhaseIdealLoop::pin_array_access_nodes_dependent_on(Node* ctrl) {
for (DUIterator i = ctrl->outs(); ctrl->has_out(i); i++) {
Node* use = ctrl->out(i);
if (!use->depends_only_on_test()) {
continue;
}
Node* pinned_clone = use->pin_array_access_node();
if (pinned_clone != nullptr) {
register_new_node(pinned_clone, get_ctrl(use));
_igvn.replace_node(use, pinned_clone);
test/hotspot/jtreg/compiler/rangechecks/TestArrayAccessAboveRCAfterSinking.java line 55:
> 53: test1(allTrue, array, -1, true, 0);
> 54: test2(allTrue, array, -1, true, 0);
> 55: } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) {
Is there a reason both are together in the same `try-catch` block? It seems like if the first throws, then the second is never executed (and may as well be removed), or we only expect the second to throw, and so the first one should be outside the bock. What do you think?
-------------
Changes requested by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17635#pullrequestreview-1867198436
PR Review Comment: https://git.openjdk.org/jdk/pull/17635#discussion_r1481124507
PR Review Comment: https://git.openjdk.org/jdk/pull/17635#discussion_r1481120156
PR Review Comment: https://git.openjdk.org/jdk/pull/17635#discussion_r1481093346
PR Review Comment: https://git.openjdk.org/jdk/pull/17635#discussion_r1481083946
PR Review Comment: https://git.openjdk.org/jdk/pull/17635#discussion_r1481081474
PR Review Comment: https://git.openjdk.org/jdk/pull/17635#discussion_r1481080736
PR Review Comment: https://git.openjdk.org/jdk/pull/17635#discussion_r1481130176
More information about the hotspot-compiler-dev
mailing list