From my comment in https://github.com/openjdk/jdk/pull/22666

To start with, what is depends_only_on_test? Given a node n with the control input c, if c can be deduced from c' and n->depends_only_on_test() == true, then we can rewire the control input of n to c'. This means that depends_only_on_test does not mean that the node depends on a test, it means that the node depends on the test that is its control input.

For example:

if (y != 0) {
    if (x > 0) {
        if (y != 0) {
            x / y;
        }
    }
}
Then x/y depends_only_on_test because its control input is the test y != 0. Then, we can rewire the control input of the division to the outer y != 0, resulting in:

if (y != 0) {
    x / y;
    if (x > 0) {
    }
}
On the other hand, consider this case:

if (x > 0) {
    if (y != 0) {
        if (x > 0) {
            x / y;
        }
    }
}
Then x/y does not depends_only_on_test because its control input is the test x > 0 which is unrelated, we can see that if we rewire the division to the outer x > 0 test, the division floats above the actual test y != 0. This means that depends_only_on_test is a dynamic property of a node, and not a static property of the division operation. It can change when we transform the graph and it can be different for different nodes of the same kind.

So, what is the issue in JDK-8257822? When the zero check of the division is split through Phi but the division is not, it is wired to the merge point, not the tests themselves. This means that the division no longer depends_only_on_test and should return false. However, it still reports that it depends_only_on_test, which makes PhaseIdealLoop::dominated_by move it out of the loop when its control input is moved.

The fix was to make PhaseIdealLoop::dominated_by treat a division as if it does not depends_only_on_test when we cannot prove that the divisor is non-zero. This fixed the issue in that particular instance, as it achieved the same result as an actual correct fix of making depends_only_on_test return false. However, the node still reports itself as depends_only_on_test, and that opens more opportunities of miscompilation.

One important consideration regarding nodes that depends_only_on_test is that we have to move them from a test to an equivalent test if we want to keep its property of depends_only_on_test. Look at the previous example:

if (y != 0) {
    if (x > 0) {
        if (y != 0) {
            x / y;
        }
    }
}
If we want to keep x/y being depends_only_on_test, we have to move it to the outer y != 0, and not to the x > 0, because in that case:

if (y != 0) {
    if (x > 0) {
        x / y;
    }
}
It can easily be seen that the division now does not depends_only_on_test. The fix for JDK-8257822 violated this, which resulted in more cases when the division mistakenly report depends_only_on_test.

For example:

for () {
    if (y != 0) {
        x / y;
    }
}
Without the fix, x/y will be moved out of the loop together with the test y != 0, and as it still has y != 0 as its control input, it is correct when reporting itself as depends_only_on_test. However, with the fix:

if (y != 0) {
}
for () {
    x / y;
}
Now x/y has its control input being the loop head, but it still depends_only_on_test. This is wrong and opens more chances of miscompilation.

For the case in this issue, it is precisely caused by this wrong approach:

for () {
    if (y != 0) {
        1 / y;
        if rangecheck {
            if (y != 0) {
                2 / y;
            }
        }
    }
}
Because IfNode::dominated_by checks for s->depends_only_on_test() && igvn->no_dependent_zero_check(s), we rewire 2 / y to the rangecheck. This makes it wrongly report itself as depends_only_on_test when its control input is a rangecheck, which is unrelated, which leads to this issue. Without this wrong approach, 2 / y will be correctly rewired to the outer y != 0, and there would be no mistaken hoist of 2 / y here.

As a result, this will inevitably converge to us doing s->depends_only_on_test() && igvn->no_dependent_zero_check(s) at every place depends_only_on_test being used, which is really equivalent to divisions being not depends_only_on_test. And this would be a safe conservative fix: to make division not depends_only_on_test, which effectively pins them to where they are created.

However, it is still not enough. The core issue here is that a node being disconnected from its test but still reporting itself as depends_only_on_test, this applies for Div, Mod, ConstraintCast, .... For example:

int x;
if (b) {} else {}
if (x > 3) {
    cast(x, [4, max_int];
}
Then if the test x > 3 is split through the region, but the CastIINode is not, then its control input would be the merge point, which is not the test it depends on, but the node still reports itself as depends_only_on_test. The difference is that a division will immediately complain when its divisor is 0, while a CastIINode will not complain anything when the value of the input does not actually lie inside the range of the CastIINode. I have tried adding a flag that verifies this and it seems there are multiple failures #22880 .

I have skimmed through the code and it seems we do actually pin the node when the new control input is not equivalent to the old control input, but only for array accesses? I believe this should be applied to all depends_only_on_test nodes, we need to pin all of them when they are no longer directly connected to their tests.