Bug 95649 - [11 Regression] ICE during GIMPLE pass: cunroll since r11-1146-g1396fa5b91cfa0b3
Summary: [11 Regression] ICE during GIMPLE pass: cunroll since r11-1146-g1396fa5b91cfa0b3
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 11.0
: P1 normal
Target Milestone: 11.0
Assignee: Aldy Hernandez
URL:
Keywords: ice-on-valid-code
: 95653 (view as bug list)
Depends on:
Blocks: yarpgen
  Show dependency treegraph
 
Reported: 2020-06-12 05:45 UTC by Vsevolod Livinskii
Modified: 2021-11-01 23:07 UTC (History)
10 users (show)

See Also:
Host:
Target:
Build:
Known to work: 10.1.0
Known to fail: 11.0
Last reconfirmed: 2020-06-12 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Livinskii 2020-06-12 05:45:48 UTC
Error:
>$ g++ -O2 -c func.cpp
during GIMPLE pass: cunroll
func.cpp: In function ‘void test()’:
func.cpp:4:6: internal compiler error: Segmentation fault
    4 | void test() {
      |      ^~~~
0x1102483 crash_signal
	gcc/toplev.c:328
0x1b58f3b loop_containing_stmt
	gcc/tree-ssa-loop.h:76
0x1b58f3b chrec_contains_symbols_defined_in_loop
	gcc/tree-chrec.c:1009
0x1b59248 chrec_contains_symbols_defined_in_loop
	gcc/tree-chrec.c:1026
0x1b59248 chrec_contains_symbols_defined_in_loop
	gcc/tree-chrec.c:1026
0x1b59248 chrec_contains_symbols_defined_in_loop
	gcc/tree-chrec.c:1026
0x1b593cb chrec_contains_symbols_defined_in_loop(tree_node const*, unsigned int)
	gcc/tree-chrec.c:1039
0x11dfff3 compute_overall_effect_of_inner_loop(loop*, tree_node*)
	gcc/tree-scalar-evolution.c:473
0x11dfff3 compute_overall_effect_of_inner_loop(loop*, tree_node*)
	gcc/tree-scalar-evolution.c:447
0x11de08c analyze_scalar_evolution_1
	gcc/tree-scalar-evolution.c:1960
0x11deb9d analyze_scalar_evolution(loop*, tree_node*)
	gcc/tree-scalar-evolution.c:2038
0x11df04f instantiate_scev_name
	gcc/tree-scalar-evolution.c:2335
0x11df04f instantiate_scev_r
	gcc/tree-scalar-evolution.c:2636
0x11df1ca instantiate_scev_binary
	gcc/tree-scalar-evolution.c:2466
0x11df1ca instantiate_scev_r
	gcc/tree-scalar-evolution.c:2649
0x11ded9e instantiate_scev_poly
	gcc/tree-scalar-evolution.c:2409
0x11ded9e instantiate_scev_r
	gcc/tree-scalar-evolution.c:2641
0x11dfde5 instantiate_scev(edge_def*, loop*, tree_node*)
	gcc/tree-scalar-evolution.c:2718
0x127743a instantiate_parameters
	gcc/tree-scalar-evolution.h:63
0x127743a idx_infer_loop_bounds
	gcc/tree-ssa-loop-niter.c:3612
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.

Reproducer:
extern unsigned short var_5;
extern int var_8, var_9;
extern short arr_7[];
void test() {
  for (; 0 < (char)var_5;)
    for (int a(var_9 ? var_5 : 0); a < 3002972621U + 1291994699;
         a += 19499 - 19497)
      for (long b(var_8); b; b += 4)
        arr_7[a * b] = 0;
}

GCC version:
11.0.0 (87af4f40453a9c84363bde5d9a58466de7fbee2e)
Comment 1 Bill Seurer 2020-06-14 14:05:51 UTC
I am seeing something slightly different that started with the same revision on powerpc64 as well.  It is preventing several of the spec2006 tests from compiling.

See pr95662.
Comment 2 Aldy Hernandez 2020-06-15 17:20:48 UTC
Here is another testcase pulled from pr95653 which I'm marking as a duplicate of this one.

This one needs -O2 -fno-tree-scev-cprop to reproduce.

char b (void);
char *d;
int e;
int f;
void
g (char *h)
{
  while (d)
    {
      long i = b ();
      if (h + i > d)
	break;
      if (f > 0 || e)
	do
	  *h++ = *h;
	while (--i);
    }
}
Comment 3 Aldy Hernandez 2020-06-15 17:21:14 UTC
Mine.
Comment 4 Aldy Hernandez 2020-06-15 17:21:59 UTC
*** Bug 95653 has been marked as a duplicate of this bug. ***
Comment 5 Aldy Hernandez 2020-06-15 18:12:40 UTC
The problem here is that the code in propagate_into_phi_args() was previously in evrp but is now in the engine itself and affecting all clients.

CCP and copyprop are two of these clients.  The subst & fold engine is changing the CFG (phis in this case) and neither pass is expecting it.  A hack showing that it's the PHI propagation is attached below.

We could make these passes clean-up the CFG for example, but that may be heavy handed if they're supposed to be lightweight ??.  Instead we could either move propagate_into_phi_args back into evrp, or only run it if a CHANGE_CFG_FLAG or somesuch is passed to the engine constructor.

I'll work on a patch.

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

NOT A FIX:

diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c
index 4fda296ef9e..249867d8633 100644
--- a/gcc/tree-ssa-propagate.c
+++ b/gcc/tree-ssa-propagate.c
@@ -1228,7 +1228,7 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
        }
     }

-  substitute_and_fold_engine->propagate_into_phi_args (bb);
+  //substitute_and_fold_engine->propagate_into_phi_args (bb);

   return NULL;
 }
Comment 6 Jeffrey A. Law 2020-06-15 19:11:40 UTC
What precisely is causing the problem?  Are we doing something like mucking up the loop closed ssa form?  Or do these passes have internal data that's invalidated by the PHI changes?  Or something else?

Hard to know what to recommend here without  more info.
Comment 7 Aldy Hernandez 2020-06-16 11:56:46 UTC
Hmmm, previous code propagating into PHI args had:

-         if (vr->singleton_p (&val) && may_propagate_copy (arg, val))
-           propagate_value (use_p, val);

We seem to have lost the check for constants, and we're now propagating any old value from get_value_range-- which may be another SSA name:

+         if (val && may_propagate_copy (arg, val))
+           propagate_value (use_p, val);


Testing the following:

diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c
index 4fda296ef9e..01ee7fd33eb 100644
--- a/gcc/tree-ssa-propagate.c
+++ b/gcc/tree-ssa-propagate.c
@@ -1035,7 +1035,8 @@ substitute_and_fold_engine::propagate_into_phi_args (basic_block bb)
              || virtual_operand_p (arg))
            continue;
          tree val = get_value (arg, phi);
-         if (val && may_propagate_copy (arg, val))
+         if (val && is_gimple_min_invariant (val)
+             && may_propagate_copy (arg, val))
            propagate_value (use_p, val);
        }
     }
Comment 8 Jeffrey A. Law 2020-06-16 14:23:46 UTC
I still don't understand why propagating one SSA_NAME for another is causing headaches later though. 

I don't see anything fundamentally wrong with your patch and it restores previous behavior since singleton_p would only be true for a constant, so consider it pre-approved.
Comment 9 GCC Commits 2020-06-16 18:51:41 UTC
The master branch has been updated by Aldy Hernandez <aldyh@gcc.gnu.org>:

https://gcc.gnu.org/g:8fb4d1d58362b77da78c09740c6b5562124a369e

commit r11-1395-g8fb4d1d58362b77da78c09740c6b5562124a369e
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Tue Jun 16 13:43:57 2020 +0200

    Fix pasto in the substitute_and_fold_engine merge with evrp.
    
    The original code only propagated into PHI arguments if the value was
    a constant.  This behavior was lost in the conversion, allowing
    any value (SSAs for instance) to be propagated into PHIs.
    
    gcc/ChangeLog:
    
            PR tree-optimization/95649
            * tree-ssa-propagate.c (propagate_into_phi_args): Do not propagate unless
            value is a constant.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/tree-ssa/pr95649.C: New test.
            * gcc.dg/tree-ssa/pr95649.c: New test.
Comment 10 Aldy Hernandez 2020-06-16 18:59:38 UTC
(In reply to Jeffrey A. Law from comment #8)
> I still don't understand why propagating one SSA_NAME for another is causing
> headaches later though. 

TBH, I don't either.

When I compile the .c test in this PR with the following options:

-O2 -fno-tree-scev-cprop -fdisable-tree-ccp1  -fdisable-tree-ccp2 -fdisable-tree-copyprop1 -fdisable-tree-evrp -fdisable-tree-vrp1 -fdisable-tree-ccp1 -fdisable-tree-ccp2 -fdisable-tree-ccp3

...I still get an ICE, but there is no change in the IL between trunk without the offending patch, and with the patch that broke the test.  This would indicate that there is some on-the-side structure that is being altered by the phi argument propagation.

I did notice that the IL *does* change in the middle of one of the copyprop (or ccp?) passes, but it gets cleaned up to whatever was there before.

Later in SCEV, we ICE while trying to look at the SSA_NAME_DEF_STMT of an SSA which no longer exists in the IL.  This happens in chrec_contains_symbols_defined_in_loop().  A wild guess is that the loop info is getting mucked up somehow.

I can investigate more deeply if desired, but since this was a silly typo, I'm gonna leave it alone for now.

Closing as fixed in mainline.