diff mbox

[PR69067] Remove assert in get_def_bb_for_const

Message ID 574AE27B.7060205@mentor.com
State New
Headers show

Commit Message

Tom de Vries May 29, 2016, 12:37 p.m. UTC 
Hi,

this patch fixes graphite PR69067, a 6/7 regression.


I.

Consider the following test-case, compiled with -O1 -floop-nest-optimize 
-flto:
...
int a1, c1, cr, kt;
int aa[2];

int
ce (void)
{
   while (a1 < 1) {
     int g8;
     for (g8 = 0; g8 < 3; ++g8)
       if (c1 != 0)
         cr = aa[a1 * 2] = kt;
     for (c1 = 0; c1 < 2; ++c1)
       aa[c1] = cr;
     ++a1;
   }
   return 0;
}

int
main (void)
{
   return ce (aa);
}
...

At graphite0, there's a loop with header bb4, which conditionally 
executes bb 5:
...
   <bb 3>:

   <bb 4>:
   # g8_39 = PHI <0(13), g8_19(3)>
   # cr_lsm.1_4 = PHI <cr_lsm.1_21(13), cr_lsm.1_33(3)>
   # cr_lsm.2_22 = PHI <cr_lsm.2_23(13), cr_lsm.2_11(3)>
   if (c1_lsm.3_35 != 0)
     goto <bb 5>;
   else
     goto <bb 6>;

   <bb 5>:
   aa[_3] = 0;

   <bb 6>:
   # cr_lsm.1_33 = PHI <cr_lsm.1_4(4), 0(5)>
   # cr_lsm.2_11 = PHI <cr_lsm.2_22(4), 1(5)>
   g8_19 = g8_39 + 1;
   if (g8_19 <= 2)
     goto <bb 3>;
   else
     goto <bb 7>;
...


II.

The graphite transformation moves the condition '(P_35 <= -1 || P_35 >= 
1)' out of the loop:
...
[scheduler] original ast:
{
   for (int c0 = 0; c0 <= 2; c0 += 1) {
     S_4(c0);
     if (P_35 <= -1 || P_35 >= 1)
       S_5(c0);
     S_6(c0);
   }
   S_7();
   for (int c0 = 0; c0 <= 1; c0 += 1)
     S_8(c0);
}

[scheduler] AST generated by isl:
{
   if (P_35 <= -1) {
     for (int c0 = 0; c0 <= 2; c0 += 1)
       S_5(c0);
   } else if (P_35 >= 1)
     for (int c0 = 0; c0 <= 2; c0 += 1)
       S_5(c0);
   for (int c0 = 0; c0 <= 2; c0 += 1) {
     S_4(c0);
     S_6(c0);
   }
   S_7();
   for (int c0 = 0; c0 <= 1; c0 += 1)
     S_8(c0);
}
...

When instantiating the ast back to gimple, we run into an assert:
...
pr-graphite-4.c: In function ‘ce’:
pr-graphite-4.c:5:1: internal compiler error: in get_def_bb_for_const, 
at graphite-isl-ast-to-gimple.c:1795
  ce()
  ^
...


III.

What happens is the following: in copy_cond_phi_args we try to copy the 
arguments of phi in bb6 to the arguments of new_phi in bb 46
...
(gdb) call debug_gimple_stmt (phi)
cr_lsm.1_33 = PHI <cr_lsm.1_4(4), 0(5)>
(gdb) call debug_gimple_stmt (new_phi)
cr_lsm.1_62 = PHI <(28), (47)>
...

While handling the '0' phi argument in add_phi_arg_for_new_expr we 
trigger this bit of code and call get_def_bb_for_const with bb.index == 
46 and old_bb.index == 5:
...
   /* If the corresponding def_bb could not be found the entry will
      be NULL.  */
   if (TREE_CODE (old_phi_args[i]) == INTEGER_CST)
     def_pred[i]
       = get_def_bb_for_const (new_bb,
                               gimple_phi_arg_edge (phi, i)->src);
...

Neither of the two copies of bb 5 dominates bb 46, so we run into the 
assert at the end:
...
/* Returns a basic block that could correspond to where a constant was
   defined in the original code.  In the original code OLD_BB had the
   definition, we need to find which basic block out of the copies of
   old_bb, in the new region, should a definition correspond to if it
   has to reach BB.  */

basic_block translate_isl_ast_to_gimple::
get_def_bb_for_const (basic_block bb, basic_block old_bb) const
{
   vec <basic_block> *bbs = region->copied_bb_map->get (old_bb);

   if (!bbs || bbs->is_empty ())
     return NULL;

   if (1 == bbs->length ())
     return (*bbs)[0];

   int i;
   basic_block b1 = NULL, b2;
   FOR_EACH_VEC_ELT (*bbs, i, b2)
     {
       if (b2 == bb)
         return bb;

       /* BB and B2 are in two unrelated if-clauses.  */
       if (!dominated_by_p (CDI_DOMINATORS, bb, b2))
         continue;

       /* Compute the nearest dominator.  */
       if (!b1 || dominated_by_p (CDI_DOMINATORS, b2, b1))
         b1 = b2;
     }

   gcc_assert (b1);
   return b1;
}
...


IV.

Attached patch fixes this by removing the assert.

Bootstrapped and reg-tested on x86_64.

OK for trunk, 6-branch?

Thanks,
- Tom

Comments

Richard Biener May 30, 2016, 7:21 a.m. UTC | #1 
On Sun, 29 May 2016, Tom de Vries wrote:

> Hi,
> 
> this patch fixes graphite PR69067, a 6/7 regression.
> 
> 
> I.
> 
> Consider the following test-case, compiled with -O1 -floop-nest-optimize
> -flto:
> ...
> int a1, c1, cr, kt;
> int aa[2];
> 
> int
> ce (void)
> {
>   while (a1 < 1) {
>     int g8;
>     for (g8 = 0; g8 < 3; ++g8)
>       if (c1 != 0)
>         cr = aa[a1 * 2] = kt;
>     for (c1 = 0; c1 < 2; ++c1)
>       aa[c1] = cr;
>     ++a1;
>   }
>   return 0;
> }
> 
> int
> main (void)
> {
>   return ce (aa);
> }
> ...
> 
> At graphite0, there's a loop with header bb4, which conditionally executes bb
> 5:
> ...
>   <bb 3>:
> 
>   <bb 4>:
>   # g8_39 = PHI <0(13), g8_19(3)>
>   # cr_lsm.1_4 = PHI <cr_lsm.1_21(13), cr_lsm.1_33(3)>
>   # cr_lsm.2_22 = PHI <cr_lsm.2_23(13), cr_lsm.2_11(3)>
>   if (c1_lsm.3_35 != 0)
>     goto <bb 5>;
>   else
>     goto <bb 6>;
> 
>   <bb 5>:
>   aa[_3] = 0;
> 
>   <bb 6>:
>   # cr_lsm.1_33 = PHI <cr_lsm.1_4(4), 0(5)>
>   # cr_lsm.2_11 = PHI <cr_lsm.2_22(4), 1(5)>
>   g8_19 = g8_39 + 1;
>   if (g8_19 <= 2)
>     goto <bb 3>;
>   else
>     goto <bb 7>;
> ...
> 
> 
> II.
> 
> The graphite transformation moves the condition '(P_35 <= -1 || P_35 >= 1)'
> out of the loop:
> ...
> [scheduler] original ast:
> {
>   for (int c0 = 0; c0 <= 2; c0 += 1) {
>     S_4(c0);
>     if (P_35 <= -1 || P_35 >= 1)
>       S_5(c0);
>     S_6(c0);
>   }
>   S_7();
>   for (int c0 = 0; c0 <= 1; c0 += 1)
>     S_8(c0);
> }
> 
> [scheduler] AST generated by isl:
> {
>   if (P_35 <= -1) {
>     for (int c0 = 0; c0 <= 2; c0 += 1)
>       S_5(c0);
>   } else if (P_35 >= 1)
>     for (int c0 = 0; c0 <= 2; c0 += 1)
>       S_5(c0);
>   for (int c0 = 0; c0 <= 2; c0 += 1) {
>     S_4(c0);
>     S_6(c0);
>   }
>   S_7();
>   for (int c0 = 0; c0 <= 1; c0 += 1)
>     S_8(c0);
> }
> ...
> 
> When instantiating the ast back to gimple, we run into an assert:
> ...
> pr-graphite-4.c: In function ‘ce’:
> pr-graphite-4.c:5:1: internal compiler error: in get_def_bb_for_const, at
> graphite-isl-ast-to-gimple.c:1795
>  ce()
>  ^
> ...
> 
> 
> III.
> 
> What happens is the following: in copy_cond_phi_args we try to copy the
> arguments of phi in bb6 to the arguments of new_phi in bb 46
> ...
> (gdb) call debug_gimple_stmt (phi)
> cr_lsm.1_33 = PHI <cr_lsm.1_4(4), 0(5)>
> (gdb) call debug_gimple_stmt (new_phi)
> cr_lsm.1_62 = PHI <(28), (47)>
> ...
> 
> While handling the '0' phi argument in add_phi_arg_for_new_expr we trigger
> this bit of code and call get_def_bb_for_const with bb.index == 46 and
> old_bb.index == 5:
> ...
>   /* If the corresponding def_bb could not be found the entry will
>      be NULL.  */
>   if (TREE_CODE (old_phi_args[i]) == INTEGER_CST)
>     def_pred[i]
>       = get_def_bb_for_const (new_bb,
>                               gimple_phi_arg_edge (phi, i)->src);
> ...
> 
> Neither of the two copies of bb 5 dominates bb 46, so we run into the assert
> at the end:
> ...
> /* Returns a basic block that could correspond to where a constant was
>   defined in the original code.  In the original code OLD_BB had the
>   definition, we need to find which basic block out of the copies of
>   old_bb, in the new region, should a definition correspond to if it
>   has to reach BB.  */
> 
> basic_block translate_isl_ast_to_gimple::
> get_def_bb_for_const (basic_block bb, basic_block old_bb) const
> {
>   vec <basic_block> *bbs = region->copied_bb_map->get (old_bb);
> 
>   if (!bbs || bbs->is_empty ())
>     return NULL;
> 
>   if (1 == bbs->length ())
>     return (*bbs)[0];
> 
>   int i;
>   basic_block b1 = NULL, b2;
>   FOR_EACH_VEC_ELT (*bbs, i, b2)
>     {
>       if (b2 == bb)
>         return bb;
> 
>       /* BB and B2 are in two unrelated if-clauses.  */
>       if (!dominated_by_p (CDI_DOMINATORS, bb, b2))
>         continue;
> 
>       /* Compute the nearest dominator.  */
>       if (!b1 || dominated_by_p (CDI_DOMINATORS, b2, b1))
>         b1 = b2;
>     }
> 
>   gcc_assert (b1);
>   return b1;
> }
> ...
> 
> 
> IV.
> 
> Attached patch fixes this by removing the assert.
> 
> Bootstrapped and reg-tested on x86_64.
> 
> OK for trunk, 6-branch?

Ok.

Thanks,
Richard.

> Thanks,
> - Tom
>
diff mbox

Patch

Remove assert in get_def_bb_for_const

2016-05-29  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/69067
	* graphite-isl-ast-to-gimple.c (get_def_bb_for_const): Remove assert.

	* gcc.dg/graphite/pr69067.c: New test.

---
 gcc/graphite-isl-ast-to-gimple.c        |  1 -
 gcc/testsuite/gcc.dg/graphite/pr69067.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/gcc/graphite-isl-ast-to-gimple.c b/gcc/graphite-isl-ast-to-gimple.c
index 049a4c5..ff1d91f 100644
--- a/gcc/graphite-isl-ast-to-gimple.c
+++ b/gcc/graphite-isl-ast-to-gimple.c
@@ -1792,7 +1792,6 @@  get_def_bb_for_const (basic_block bb, basic_block old_bb) const
 	b1 = b2;
     }
 
-  gcc_assert (b1);
   return b1;
 }
 
diff --git a/gcc/testsuite/gcc.dg/graphite/pr69067.c b/gcc/testsuite/gcc.dg/graphite/pr69067.c
new file mode 100644
index 0000000..d767381d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/graphite/pr69067.c
@@ -0,0 +1,28 @@ 
+/* { dg-do link } */
+/* { dg-options " -O1 -floop-nest-optimize" } */
+/* { dg-additional-options "-flto" { target lto } } */
+
+int a1, c1, cr, kt;
+int aa[2];
+
+int
+ce (void)
+{
+  while (a1 < 1)
+    {
+      int g8;
+      for (g8 = 0; g8 < 3; ++g8)
+	if (c1 != 0)
+	  cr = aa[a1 * 2] = kt;
+      for (c1 = 0; c1 < 2; ++c1)
+	aa[c1] = cr;
+      ++a1;
+    }
+  return 0;
+}
+
+int
+main (void)
+{
+  return ce (aa);
+}