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

[Liu Xin]

Hi, Roland,

Thank you so much! Please see the comments inline.

On Thu, Aug 29, 2019 at 5:21 AM Roland Westrelin <rwestrel at redhat.com>
wrote:

>
> 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.
>
>
for 2),  yes, it's definitely an illegal transformation.
but I feel it's because it moves in code right after inner loop exit.
New code could be 1) cause data dependency  2) change control flow, eg.
return early

Do you think my comment is explanatory?
https://cr.openjdk.java.net/~xliu/8229450/03/webrev/

I just did cosmetic things for your patch.  Could you review and sponsor
it?
I also verified it using hotspot:tier1.

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.


thanks for the IfNode::search_identical() thing.
Let's forget about search_identical()  this time because it won't hit a
bug.


>

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.
>
> Thanks a lot! Those are very valuable take-away for me too.

Apparently, I am learning c2 in a hard way.  My direction was wrong.
Tweaking original program is difficult or even impossible sometimes.
I feel your approach 'model a chain of events' is more scientific. Could
you educate me more?

1. Is it a mental "chain of event", or you can dump it from JVM?

2. What's your definition of 'event' here?  Do you mean an event is an
individual optimization?
I did write down a sequence of optimizations for this case, but I still
can't write a testcase...
You are so amazing! I know how to make up code, but I don't know what event
I want to trigger.
Could you tell me what  'chain of events' leads you to your test1?


Thanks,
--lx



> 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) {
>     }
> }
>