Fix latent bug in replace_uses_by

The following fixes an ICE I got when building libjava with a local
patch.  This causes us to substitute &MEM[&a, 5] into MEM[_3, 0]
to MEM[&MEM[&a, 5], 0] and then asking stmt_ends_bb_p which doesn't
expect such bogus MEM_REFs.  The MEM_REF is canonicalized by
calling fold_stmt on it later, but the fix is of course to move
the marking of altered BBs before doing the actual substitution
(only then we are sure to catch all previous bb-ending stmts).

I also noticed we don't verify MEM_REFs on LHSs.

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied
to trunk and branch (it's a regression uncovered by the fix for PR60115).

Richard.

2014-02-20  Richard Biener  <rguenther@suse.de>

	* tree-cfg.c (replace_uses_by): Mark altered BBs before
	doing the substitution.
	(verify_gimple_assign_single): Also verify bare MEM_REFs
	on the lhs.

On Thu, Feb 20, 2014 at 10:51 PM, Richard Biener <rguenther@suse.de> wrote:
>
> The following fixes an ICE I got when building libjava with a local
> patch.  This causes us to substitute &MEM[&a, 5] into MEM[_3, 0]
> to MEM[&MEM[&a, 5], 0] and then asking stmt_ends_bb_p which doesn't
> expect such bogus MEM_REFs.  The MEM_REF is canonicalized by
> calling fold_stmt on it later, but the fix is of course to move
> the marking of altered BBs before doing the actual substitution
> (only then we are sure to catch all previous bb-ending stmts).
>
> I also noticed we don't verify MEM_REFs on LHSs.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, applied
> to trunk and branch (it's a regression uncovered by the fix for PR60115).
>
> Richard.
>
> 2014-02-20  Richard Biener  <rguenther@suse.de>
>
>         * tree-cfg.c (replace_uses_by): Mark altered BBs before
>         doing the substitution.
>         (verify_gimple_assign_single): Also verify bare MEM_REFs
>         on the lhs.
>
> Index: gcc/tree-cfg.c
> ===================================================================
> --- gcc/tree-cfg.c      (revision 207936)
> +++ gcc/tree-cfg.c      (working copy)
> @@ -1677,6 +1677,11 @@ replace_uses_by (tree name, tree val)
>
>    FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name)
>      {
> +      /* Mark the block if we change the last stmt in it.  */
> +      if (cfgcleanup_altered_bbs
> +         && stmt_ends_bb_p (stmt))
> +       bitmap_set_bit (cfgcleanup_altered_bbs, gimple_bb (stmt)->index);
> +
>        FOR_EACH_IMM_USE_ON_STMT (use, imm_iter)
>          {
>           replace_exp (use, val);
> @@ -1701,11 +1706,6 @@ replace_uses_by (tree name, tree val)
>           gimple orig_stmt = stmt;
>           size_t i;
>
> -         /* Mark the block if we changed the last stmt in it.  */
> -         if (cfgcleanup_altered_bbs
> -             && stmt_ends_bb_p (stmt))
> -           bitmap_set_bit (cfgcleanup_altered_bbs, gimple_bb (stmt)->index);
> -
Hi Richard,
I also noticed this with local patch, but is it OK just to move above
code after fold_stmt? In other words, does phi node matter (according
to comments before cfgcleanup_altered_bbs)?

Thanks in advance.


>           /* FIXME.  It shouldn't be required to keep TREE_CONSTANT
>              on ADDR_EXPRs up-to-date on GIMPLE.  Propagation will
>              only change sth from non-invariant to invariant, and only
> @@ -3986,7 +3986,9 @@ verify_gimple_assign_single (gimple stmt
>        return true;
>      }
>
> -  if (handled_component_p (lhs))
> +  if (handled_component_p (lhs)
> +      || TREE_CODE (lhs) == MEM_REF
> +      || TREE_CODE (lhs) == TARGET_MEM_REF)
>      res |= verify_types_in_gimple_reference (lhs, true);
>
>    /* Special codes we cannot handle via their class.  */

On Fri, 21 Feb 2014, Bin.Cheng wrote:

> On Thu, Feb 20, 2014 at 10:51 PM, Richard Biener <rguenther@suse.de> wrote:
> >
> > The following fixes an ICE I got when building libjava with a local
> > patch.  This causes us to substitute &MEM[&a, 5] into MEM[_3, 0]
> > to MEM[&MEM[&a, 5], 0] and then asking stmt_ends_bb_p which doesn't
> > expect such bogus MEM_REFs.  The MEM_REF is canonicalized by
> > calling fold_stmt on it later, but the fix is of course to move
> > the marking of altered BBs before doing the actual substitution
> > (only then we are sure to catch all previous bb-ending stmts).
> >
> > I also noticed we don't verify MEM_REFs on LHSs.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, applied
> > to trunk and branch (it's a regression uncovered by the fix for PR60115).
> >
> > Richard.
> >
> > 2014-02-20  Richard Biener  <rguenther@suse.de>
> >
> >         * tree-cfg.c (replace_uses_by): Mark altered BBs before
> >         doing the substitution.
> >         (verify_gimple_assign_single): Also verify bare MEM_REFs
> >         on the lhs.
> >
> > Index: gcc/tree-cfg.c
> > ===================================================================
> > --- gcc/tree-cfg.c      (revision 207936)
> > +++ gcc/tree-cfg.c      (working copy)
> > @@ -1677,6 +1677,11 @@ replace_uses_by (tree name, tree val)
> >
> >    FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name)
> >      {
> > +      /* Mark the block if we change the last stmt in it.  */
> > +      if (cfgcleanup_altered_bbs
> > +         && stmt_ends_bb_p (stmt))
> > +       bitmap_set_bit (cfgcleanup_altered_bbs, gimple_bb (stmt)->index);
> > +
> >        FOR_EACH_IMM_USE_ON_STMT (use, imm_iter)
> >          {
> >           replace_exp (use, val);
> > @@ -1701,11 +1706,6 @@ replace_uses_by (tree name, tree val)
> >           gimple orig_stmt = stmt;
> >           size_t i;
> >
> > -         /* Mark the block if we changed the last stmt in it.  */
> > -         if (cfgcleanup_altered_bbs
> > -             && stmt_ends_bb_p (stmt))
> > -           bitmap_set_bit (cfgcleanup_altered_bbs, gimple_bb (stmt)->index);
> > -
> Hi Richard,
> I also noticed this with local patch, but is it OK just to move above
> code after fold_stmt? In other words, does phi node matter (according
> to comments before cfgcleanup_altered_bbs)?

PHI node doesn't matter but doesn't trigger stmt_ends_bb_p anyway.
It's better to do before the replacement because a stmt that may
have been stmt_ends_bb_p before the replacement might not be
after it (and thus we'd miss a cfgcleanup opportunity to merge
two blocks).

Richard.

> Thanks in advance.
> 
> 
> >           /* FIXME.  It shouldn't be required to keep TREE_CONSTANT
> >              on ADDR_EXPRs up-to-date on GIMPLE.  Propagation will
> >              only change sth from non-invariant to invariant, and only
> > @@ -3986,7 +3986,9 @@ verify_gimple_assign_single (gimple stmt
> >        return true;
> >      }
> >
> > -  if (handled_component_p (lhs))
> > +  if (handled_component_p (lhs)
> > +      || TREE_CODE (lhs) == MEM_REF
> > +      || TREE_CODE (lhs) == TARGET_MEM_REF)
> >      res |= verify_types_in_gimple_reference (lhs, true);
> >
> >    /* Special codes we cannot handle via their class.  */
> 
> 
> 
>