On Sun, Jun 29, 2014 at 12:33 AM, Marc Glisse <marc.gli...@inria.fr> wrote:
> Hello,
>
> we currently drop clobbers on variables whose address is not taken anymore.
> However, rewrite_stmt has code to replace them with an SSA_NAME with a
> default definition (an uninitialized variable), and I believe
> rewrite_update_stmt should do the same. This allows us to warn sometimes
> (see testcase), but during the debugging I also noticed several places where
> it allowed CCP to simplify further PHIs, so this is also an optimization.
>
> In an earlier version of the patch, I was using
> get_or_create_ssa_default_def (cfun, sym);
> (I was reusing the same variable). This passed bootstrap+testsuite on all
> languages except for ada. Indeed, the compiler wanted to coalesce several
> SSA_NAMEs, including those new ones, in out-of-ssa, but couldn't. There are
> abnormal PHIs involved. Maybe it shouldn't have insisted on coalescing an
> undefined ssa_name, maybe something should have prevented us from reaching
> such a situation, but creating a new variable was the simplest workaround.
Hmm. We indeed notice "late" that the new names are used in abnormal
PHIs. Note that usually rewriting a symbol into SSA form does not
create overlapping life-ranges - but of course you are possibly introducing
those by the new use of the default definitions.
Apart from the out-of-SSA patch you proposed elsewhere a possibility
is to simply never mark undefined SSA names as
SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
as must-coalesce).
> Some things could be done to improve the error message in uninit:
> - getting the location of the variable,
> - differenciating uninitialized from clobbered,
> but that can come later.
>
> Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-unknown-linux-gnu.
>
> 2014-06-30 Marc Glisse <marc.gli...@inria.fr>
>
> PR tree-optimization/60770
> gcc/
> * tree-ssa.c (execute_update_addresses_taken): Don't drop clobbers.
> * tree-into-ssa.c (maybe_register_def): Replace clobbers with a
> default definition.
> gcc/testsuite/
> * gcc.dg/tree-ssa/pr60770-1.c: New file.
>
> --
> Marc Glisse
> Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (working copy)
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -Wall" } */
> +
> +int f(int n){
> + int*p;
> + {
> + int yyy=n;
> + p=&yyy;
> + }
> + return *p; /* { dg-warning "yyy" } */
> +}
> Index: gcc/tree-into-ssa.c
> ===================================================================
> --- gcc/tree-into-ssa.c (revision 212109)
> +++ gcc/tree-into-ssa.c (working copy)
> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
> {
> tree def = DEF_FROM_PTR (def_p);
> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>
> /* If DEF is a naked symbol that needs renaming, create a new
> name for it. */
> if (marked_for_renaming (sym))
> {
> if (DECL_P (def))
> {
> - tree tracked_var;
> -
> - def = make_ssa_name (def, stmt);
> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
sym should always be a gimple reg here (it's marked for renaming).
> + {
> + /* Replace clobber stmts with a default def. Create a new
> + variable so we don't later think we must coalesce, which
> would
> + fail with some ada abnormal PHIs. Still, we try to keep a
> + similar name so error messages make sense. */
> + unlink_stmt_vdef (stmt);
I think that's redundant with gsi_replace (note that using gsi_replace
looks dangerous here as it calls update_stmt during SSA rewrite...
that might open a can of worms).
> + gsi_replace (&gsi, gimple_build_nop (), true);
> + tree id = DECL_NAME (sym);
> + const char* name = id ? IDENTIFIER_POINTER (id) : 0;
> + tree newvar = create_tmp_var (TREE_TYPE (sym), name);
> + def = get_or_create_ssa_default_def (cfun, newvar);
So - can't you simply do
gimple_assign_set_rhs_from_tree (&gsi,
get_or_create_dda_default_def (cfun, sym));
? Thus replace x = CLOBBER; with x_3 = x_2(D);
> + }
> + else
and of course still rewrite the DEF then. IMHO the copy-propagation
you do is premature optimization.
> + def = make_ssa_name (def, stmt);
> SET_DEF (def_p, def);
>
> - tracked_var = target_for_debug_bind (sym);
> + tree tracked_var = target_for_debug_bind (sym);
> if (tracked_var)
> {
> gimple note = gimple_build_debug_bind (tracked_var, def,
> stmt);
> /* If stmt ends the bb, insert the debug stmt on the single
> non-EH edge from the stmt. */
> if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
> {
> basic_block bb = gsi_bb (gsi);
> edge_iterator ei;
> edge e, ef = NULL;
> Index: gcc/tree-ssa.c
> ===================================================================
> --- gcc/tree-ssa.c (revision 212109)
> +++ gcc/tree-ssa.c (working copy)
> @@ -1607,32 +1607,20 @@ execute_update_addresses_taken (void)
> rhs = gimple_assign_rhs1 (stmt);
> if (gimple_assign_lhs (stmt) != lhs
> && !useless_type_conversion_p (TREE_TYPE (lhs),
> TREE_TYPE (rhs)))
> rhs = fold_build1 (VIEW_CONVERT_EXPR,
> TREE_TYPE (lhs), rhs);
>
> if (gimple_assign_lhs (stmt) != lhs)
> gimple_assign_set_lhs (stmt, lhs);
>
> - /* For var ={v} {CLOBBER}; where var lost
> - TREE_ADDRESSABLE just remove the stmt. */
> - if (DECL_P (lhs)
> - && TREE_CLOBBER_P (rhs)
> - && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
> - {
> - unlink_stmt_vdef (stmt);
> - gsi_remove (&gsi, true);
> - release_defs (stmt);
> - continue;
> - }
> -
> if (gimple_assign_rhs1 (stmt) != rhs)
> {
> gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
> gimple_assign_set_rhs_from_tree (&gsi, rhs);
> }
> }
>
> else if (gimple_code (stmt) == GIMPLE_CALL)
> {
> unsigned i;
>
