RFR(XS): 8229450: C2 compilation fails with assert(found_sfpt) failed

[Roland Westrelin]

Hi,

Thanks for working on that bug.

> Please review my change to fix IfSplitBlocks for a corner case.
> JBS: https://bugs.openjdk.java.net/browse/JDK-8229450
> webrev: https://cr.openjdk.java.net/~xliu/8229450/webrev/

The fix is correct but too conservative I think. The optimization itself
is valid: the second if is redundant and should be eliminated but
control dependent data nodes shouldn't be moved in the inner strip mined
loop. That for 2 reasons: 1) it breaks verification code/strip mining
assumptions as you've found 2) it's actually an illegal transformation
because at the end of compilation the inner loop exit condition is
adjusted to only cover a subset of the iterations so the inner loop exit
condition is not longer redundant with the if that's eliminated.

I would propose this as a fix instead:

diff -r 85fbdb87baad -r c1ff28db28e7 src/hotspot/share/opto/loopopts.cpp
--- a/src/hotspot/share/opto/loopopts.cpp	Wed Aug 14 15:07:04 2019 +0200
+++ b/src/hotspot/share/opto/loopopts.cpp	Thu Aug 29 13:35:15 2019 +0200
@@ -1326,6 +1326,11 @@
         if (dom->req() > 1 && dom->in(1) == bol && prevdom->in(0) == dom) {
           // Replace the dominated test with an obvious true or false.
           // Place it on the IGVN worklist for later cleanup.
+          if (prevdom->in(0)->is_CountedLoopEnd() &&
+              prevdom->in(0)->as_CountedLoopEnd()->loopnode() != NULL &&
+              prevdom->in(0)->as_CountedLoopEnd()->loopnode()->is_strip_mined()) {
+            prevdom = prevdom->in(0)->as_CountedLoopEnd()->loopnode()->in(LoopNode::EntryControl)->as_OuterStripMinedLoop()->outer_loop_exit();
+          }
           C->set_major_progress();
           dominated_by(prevdom, n, false, true);
 #ifndef PRODUCT

Remove the redundant if but replace it by the exit of the outer strip
mined loop.

I also managed to write a test case. See below. I run it with:

java -XX:-TieredCompilation -XX:-UseOnStackReplacement -XX:-BackgroundCompilation -XX:LoopMaxUnroll=0 -XX:CompileCommand=dontinline,LoadDependsOnIfIdenticalToLoopExit::not_inlined LoadDependsOnIfIdenticalToLoopExit 

Note that removing an If if it's the branch of a dominating equivalent
If is performed twice: once during loop opts (the one that causes this
bug) and once during igvn in IfNode::search_identical(). test2() tries
to trigger the same bug but during igvn but with the current code it
fails to because the igvn code is stricter (too strict actually) because
it checks that the dominating If opcode is the same as the dominated If
so a CountedLoopEnd can't replace an If.

My own recipe for writing regression tests is to:

1) understand the failure
2) determine the chain of events that lead to the failure
3) write a test case that reproduces the chain of events from scratch,
ignoring the actual method that caused the bug in the first place
4) if I can't find out how to trigger a particular event, I go back to
the initial failure, try to understand what happens and incorporate it
in the test case

So I don't try to replicate the failing method but build my own method
from scratch from my understanding of the bug.

Roland.

public class LoadDependsOnIfIdenticalToLoopExit {
    private static int[] field = new int[1];
    private int field2;

    public static void main(String[] args) {
        LoadDependsOnIfIdenticalToLoopExit instance = new LoadDependsOnIfIdenticalToLoopExit();
        for (int i = 0; i < 20_000; i++) {
            test1(false, false);
            test1(true, true);
            test2();
        }
    }

    private static int test1(boolean flag1, boolean flag2) {
        int res = 1;
        int[] array = new int[10];
        not_inlined(array);
        int i;
        for (i = 0; i < 2000; i++) {
            res *= i;
        }

        if (flag1) {
            if (flag2) {
                res++;
            }
        }

        if (i >= 2000) {
            res *= array[0];
        }
        return res;
    }

    private static int test2() {
        int j = 2;
        for (; j < 4; j *= 2);

        int res = 1;
        int[] array = new int[10];
        not_inlined(array);
        int i;
        for (i = 1; i < 2000; i++) {
            res *= i;
        }

        if (i >= j * 500) {
            res *= array[0];
        }
        return res;
    }

    private static void not_inlined(int[] array) {
    }
}