PR middle-end/44485 (error_mark instatement)

Hi,
after function is found noreturn, we have to remove its return value.  Use of the return
value remains in the caller function body until next removal of unreachable code.  For this
reason I added code replacing SSA_NAME with error_mark.

This unfortunately triggers ICE when fixup_cfg decide to update on of the statements (this
happens when the other statement is also call).

THis patch just modify fixup_noreturn_call to remove those basic blocks proactively.
It also seems to fix the Ada problem with the same symptom.

Bootstrapped/regtested x86_64-linux, seems to make sense?

Honza

static int
foo (int si1, int si2)
{
  return si1 > 0 && si2 > 0 && si1 > -si2 || si1 < 0 && si2 < 0
    && si1 < -si2 ? : si1 + si2;
}

struct S0
{
  unsigned short f1;
};
int g_4;
struct S0 g_54 = {
  3428
};

int
func_21 (int * p_22, int * const int32p_24, unsigned p_25,
         const int * p_26);

void int324 (unsigned p_15, int * p_16, int * p_17, int * p_18)
{
  if (foo (g_4, func_21 (p_18, &g_4, 0, 0)))
    {
      for (g_54.f1; g_54.f1; g_54.f1 += 1)
        {
        }
    }
}

int
func_21 (int * p_22, int * const int32p_24, unsigned p_25,
         const int * p_26)
{
  for (0; 1; p_25 += 1)
  lbl_29:if (p_25)
      goto lbl_28;
lbl_28:for (p_25 = 0; p_25 < 9; p_25 += 1)
    if (p_25)
      goto lbl_29;
  unsigned short l_53;
  for (0; l_53; l_53 = foo)
    {
    }
  return 0;
}

	PR middle-end/44485
	* tree-cfgcleanup.c (fixup_noreturn_call): Remove basic block contianing
	use of return value of function found to be noreturn.

On Sat, Jun 12, 2010 at 12:54 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> after function is found noreturn, we have to remove its return value.  Use of the return
> value remains in the caller function body until next removal of unreachable code.  For this
> reason I added code replacing SSA_NAME with error_mark.
>
> This unfortunately triggers ICE when fixup_cfg decide to update on of the statements (this
> happens when the other statement is also call).
>
> THis patch just modify fixup_noreturn_call to remove those basic blocks proactively.
> It also seems to fix the Ada problem with the same symptom.
>
> Bootstrapped/regtested x86_64-linux, seems to make sense?



> Honza
>
> static int
> foo (int si1, int si2)
> {
>  return si1 > 0 && si2 > 0 && si1 > -si2 || si1 < 0 && si2 < 0
>    && si1 < -si2 ? : si1 + si2;
> }
>
> struct S0
> {
>  unsigned short f1;
> };
> int g_4;
> struct S0 g_54 = {
>  3428
> };
>
> int
> func_21 (int * p_22, int * const int32p_24, unsigned p_25,
>         const int * p_26);
>
> void int324 (unsigned p_15, int * p_16, int * p_17, int * p_18)
> {
>  if (foo (g_4, func_21 (p_18, &g_4, 0, 0)))
>    {
>      for (g_54.f1; g_54.f1; g_54.f1 += 1)
>        {
>        }
>    }
> }
>
> int
> func_21 (int * p_22, int * const int32p_24, unsigned p_25,
>         const int * p_26)
> {
>  for (0; 1; p_25 += 1)
>  lbl_29:if (p_25)
>      goto lbl_28;
> lbl_28:for (p_25 = 0; p_25 < 9; p_25 += 1)
>    if (p_25)
>      goto lbl_29;
>  unsigned short l_53;
>  for (0; l_53; l_53 = foo)
>    {
>    }
>  return 0;
> }
>
>        PR middle-end/44485
>        * tree-cfgcleanup.c (fixup_noreturn_call): Remove basic block contianing
>        use of return value of function found to be noreturn.
> Index: tree-cfgcleanup.c
> ===================================================================
> --- tree-cfgcleanup.c   (revision 160614)
> +++ tree-cfgcleanup.c   (working copy)
> @@ -559,18 +559,35 @@ fixup_noreturn_call (gimple stmt)
>     {
>       tree op = gimple_call_lhs (stmt);
>       gimple_call_set_lhs (stmt, NULL_TREE);
> +
>       /* We need to remove SSA name to avoid checking.

To avoid checking errors?

>         All uses are dominated by the noreturn and thus will
> -        be removed afterwards.  */
> +        be removed afterwards.
> +        We proactively remove affected non-PHI statements to avoid
> +        fixup_cfg from trying to update them and crashing.  */
>       if (TREE_CODE (op) == SSA_NAME)
>        {
>          use_operand_p use_p;
>           imm_use_iterator iter;
>          gimple use_stmt;
> +         bitmap_iterator bi;
> +         unsigned int bb_index;
> +
> +         bitmap_head blocks;
> +         bitmap_initialize (&blocks, &bitmap_default_obstack);

I know you like to replace all bitmaps by bitmap_head, but in this
case please use a bitmap and the usual BITMAP_ALLOC/FREE.

Ok with that change.

Thanks,
Richard.

>           FOR_EACH_IMM_USE_STMT (use_stmt, iter, op)
> -           FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
> -             SET_USE (use_p, error_mark_node);
> +           {
> +             if (gimple_code (use_stmt) != GIMPLE_PHI)
> +               bitmap_set_bit (&blocks, gimple_bb (use_stmt)->index);
> +             else
> +               FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
> +                 SET_USE (use_p, error_mark_node);
> +           }
> +         EXECUTE_IF_SET_IN_BITMAP (&blocks, 0, bb_index, bi)
> +           delete_basic_block (BASIC_BLOCK (bb_index));
> +         bitmap_clear (&blocks);
> +         release_ssa_name (op);
>        }
>       update_stmt (stmt);
>       changed = true;
>

Hi,

On Sat, 12 Jun 2010, Richard Guenther wrote:

> > after function is found noreturn, we have to remove its return value. 
> >  Use of the return value remains in the caller function body until 
> > next removal of unreachable code.  For this reason I added code 
> > replacing SSA_NAME with error_mark.
> >
> > This unfortunately triggers ICE when fixup_cfg decide to update on of 
> > the statements (this happens when the other statement is also call).
> >
> > THis patch just modify fixup_noreturn_call to remove those basic 
> > blocks proactively. It also seems to fix the Ada problem with the same 
> > symptom.

I think I have a better idea below.

> > +         bitmap_head blocks;
> > +         bitmap_initialize (&blocks, &bitmap_default_obstack);
> 
> I know you like to replace all bitmaps by bitmap_head, but in this
> case please use a bitmap and the usual BITMAP_ALLOC/FREE.
> 
> Ok with that change.

Actually I think it would be best (if the idea works) to replace the 
defining statement of that SSA name with a gimple_nop (and take it out of 
any BB), so that it becomes a normal valid use of an uninitialized SSA 
name, without going over any of its uses or having to adjust them, or 
allocating/freeing this bitmap.


Ciao,
Michael.