On Tue, 24 Jul 2018, Tom de Vries wrote:
> On Tue, Jul 24, 2018 at 02:34:26PM +0200, Tom de Vries wrote:
> > On 07/24/2018 01:46 PM, Jakub Jelinek wrote:
> > > On Tue, Jul 24, 2018 at 01:37:32PM +0200, Tom de Vries wrote:
> > >> Another drawback is that the fake uses confuse the unitialized warning
> > >> analysis, so that is switched off for -fkeep-vars-live.
> > >
> > > Is that really needed? I.e. can't you for the purpose of uninitialized
> > > warning analysis ignore the clobber = var uses?
> > >
> >
> > This seems to work on the test-case that failed during testing
> > (g++.dg/uninit-pred-4.C):
> > ...
> > diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
> > index 77f090bfa80..953db9ed02d 100644
> > --- a/gcc/tree-ssa-uninit.c
> > +++ b/gcc/tree-ssa-uninit.c
> > @@ -132,6 +132,9 @@ warn_uninit (enum opt_code wc, tree t, tree expr,
> > tree var,
> > if (is_gimple_assign (context)
> > && gimple_assign_rhs_code (context) == COMPLEX_EXPR)
> > return;
> > + if (gimple_assign_single_p (context)
> > + && TREE_CLOBBER_P (gimple_assign_lhs (context)))
> > + return;
> > if (!has_undefined_value_p (t))
> > return;
> > ...
> > But I don't know the pass well enough to know whether this is a
> > sufficient fix.
> >
>
> Updated and re-tested patch.
>
> > +Add artificial use for each local variable at the end of the
> > declaration scope
>
> Is this a better option description?
>
>
> OK for trunk?
>
> Thanks,
> - Tom
>
> [debug] Add fkeep-vars-live
>
> This patch adds fake uses of user variables at the point where they go out of
> scope, to keep user variables inspectable throughout the application.
>
> This approach will generate sub-optimal code: in some cases, the executable
> code will go through efforts to keep a var alive, while var-tracking can
> easily compute the value of the var from something else.
>
> Also, the compiler treats the fake use as any other use, so it may keep an
> expensive resource like a register occupied (if we could mark the use as a
> cold use or some such, we could tell optimizers that we need the value, but
> it's ok if getting the value is expensive, so it could be spilled instead of
> occupying a register).
>
> The current implementation is expected to increase register pressure, and
> therefore spilling, but we'd still expect less memory accesses then with O0.
Few comments inline.
> 2018-07-24 Tom de Vries <tdevr...@suse.de>
>
> PR debug/78685
> * cfgexpand.c (expand_use): New function.
> (expand_gimple_stmt_1): Handle TREE_CLOBBER_P as lhs of assignment.
> * common.opt (fkeep-vars-live): New option.
> * function.c (instantiate_virtual_regs): Instantiate in USEs as well.
> * gimplify.c (gimple_build_uses): New function.
> (gimplify_bind_expr): Build clobber uses for variables that don't have
> to be in memory.
> (gimplify_body): Build clobber uses for arguments.
> * tree-cfg.c (verify_gimple_assign_single): Handle TREE_CLOBBER_P as lhs
> of assignment.
> * tree-sra.c (sra_modify_assign): Same.
> * tree-ssa-alias.c (refs_may_alias_p_1): Same.
> * tree-ssa-structalias.c (find_func_aliases): Same.
> * tree-ssa-uninit.c (warn_uninit): Same.
>
> * gcc.dg/guality/pr54200-2.c: Update.
>
> ---
> gcc/cfgexpand.c | 11 ++++++++
> gcc/common.opt | 4 +++
> gcc/function.c | 5 ++--
> gcc/gimplify.c | 46
> +++++++++++++++++++++++++++-----
> gcc/testsuite/gcc.dg/guality/pr54200-2.c | 3 +--
> gcc/tree-cfg.c | 1 +
> gcc/tree-sra.c | 7 +++++
> gcc/tree-ssa-alias.c | 4 +++
> gcc/tree-ssa-structalias.c | 3 ++-
> gcc/tree-ssa-uninit.c | 3 +++
> 10 files changed, 76 insertions(+), 11 deletions(-)
>
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index d6e3c382085..e28e8ceec75 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -3533,6 +3533,15 @@ expand_clobber (tree lhs)
> }
> }
>
> +/* Expand a use of RHS. */
> +
> +static void
> +expand_use (tree rhs)
> +{
> + rtx target = expand_expr (rhs, NULL_RTX, VOIDmode, EXPAND_NORMAL);
> + emit_use (target);
> +}
> +
> /* A subroutine of expand_gimple_stmt, expanding one gimple statement
> STMT that doesn't require special handling for outgoing edges. That
> is no tailcalls and no GIMPLE_COND. */
> @@ -3632,6 +3641,8 @@ expand_gimple_stmt_1 (gimple *stmt)
> /* This is a clobber to mark the going out of scope for
> this LHS. */
> expand_clobber (lhs);
> + else if (TREE_CLOBBER_P (lhs))
> + expand_use (rhs);
> else
> expand_assignment (lhs, rhs,
> gimple_assign_nontemporal_move_p (
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 984b351cd79..a29e320ba45 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -1496,6 +1496,10 @@ fdebug-nops
> Common Report Var(flag_debug_nops) Optimization
> Insert nops if that might improve debugging of optimized code.
>
> +fkeep-vars-live
> +Common Report Var(flag_keep_vars_live) Optimization
> +Add artificial uses of local vars at end of scope.
> +
> fkeep-gc-roots-live
> Common Undocumented Report Var(flag_keep_gc_roots_live) Optimization
> ; Always keep a pointer to a live memory block
> diff --git a/gcc/function.c b/gcc/function.c
> index dee303cdbdd..b6aa1eb60d1 100644
> --- a/gcc/function.c
> +++ b/gcc/function.c
> @@ -1964,11 +1964,12 @@ instantiate_virtual_regs (void)
> {
> /* These patterns in the instruction stream can never be recognized.
> Fortunately, they shouldn't contain virtual registers either. */
> - if (GET_CODE (PATTERN (insn)) == USE
> - || GET_CODE (PATTERN (insn)) == CLOBBER
> + if (GET_CODE (PATTERN (insn)) == CLOBBER
> || GET_CODE (PATTERN (insn)) == ASM_INPUT
> || DEBUG_MARKER_INSN_P (insn))
> continue;
> + else if (GET_CODE (PATTERN (insn)) == USE)
> + instantiate_virtual_regs_in_rtx (&PATTERN (insn));
> else if (DEBUG_BIND_INSN_P (insn))
> instantiate_virtual_regs_in_rtx (INSN_VAR_LOCATION_PTR (insn));
> else
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 4a109aee27a..afe1fc836d1 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1264,6 +1264,25 @@ asan_poison_variables (hash_set<tree> *variables, bool
> poison, gimple_seq *seq_p
> }
> }
>
> +/* Return clobber uses for VARS. */
> +
> +static gimple_seq
> +gimple_build_uses (tree vars)
> +{
> + gimple_seq seq = NULL;
> +
> + for (tree var = vars; var; var = DECL_CHAIN (var))
> + {
> + if (DECL_ARTIFICIAL (var))
I think you want DECL_IGNORED_P here, artificial vars can be refered
to by debug info.
> + continue;
> +
> + gimple *stmt = gimple_build_assign (build_clobber (TREE_TYPE (var)),
> var);
> + gimple_seq_add_stmt (&seq, stmt);
> + }
> +
> + return seq;
> +}
> +
There's a single call of this function, please inline it.
> /* Gimplify a BIND_EXPR. Just voidify and recurse. */
>
> static enum gimplify_status
> @@ -1363,7 +1382,7 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
> gimplify_seq_add_stmt (&cleanup, stack_restore);
> }
>
> - /* Add clobbers for all variables that go out of scope. */
> + /* Add clobbers/uses for all variables that go out of scope. */
> for (t = BIND_EXPR_VARS (bind_expr); t ; t = DECL_CHAIN (t))
> {
> if (VAR_P (t)
> @@ -1376,14 +1395,17 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
> /* Only care for variables that have to be in memory. Others
> will be rewritten into SSA names, hence moved to the
> top-level. */
> - && !is_gimple_reg (t)
> + && (flag_keep_vars_live || !is_gimple_reg (t))
> && flag_stack_reuse != SR_NONE)
> {
> tree clobber = build_clobber (TREE_TYPE (t));
> - gimple *clobber_stmt;
> - clobber_stmt = gimple_build_assign (t, clobber);
> - gimple_set_location (clobber_stmt, end_locus);
> - gimplify_seq_add_stmt (&cleanup, clobber_stmt);
> + gimple *stmt;
> + if (is_gimple_reg (t))
> + stmt = gimple_build_assign (clobber, t);
> + else
> + stmt = gimple_build_assign (t, clobber);
> + gimple_set_location (stmt, end_locus);
> + gimplify_seq_add_stmt (&cleanup, stmt);
> }
>
> if (flag_openacc && oacc_declare_returns != NULL)
> @@ -12763,6 +12785,10 @@ gimplify_body (tree fndecl, bool do_parms)
> gimple *outer_stmt;
> gbind *outer_bind;
>
> + gimple_seq cleanup = NULL;
> + if (flag_keep_vars_live)
> + cleanup = gimple_build_uses (DECL_ARGUMENTS (fndecl));
> +
> timevar_push (TV_TREE_GIMPLIFY);
>
> init_tree_ssa (cfun);
> @@ -12798,6 +12824,14 @@ gimplify_body (tree fndecl, bool do_parms)
> /* Gimplify the function's body. */
> seq = NULL;
> gimplify_stmt (&DECL_SAVED_TREE (fndecl), &seq);
> +
> + if (cleanup)
> + {
> + gtry *gs = gimple_build_try (seq, cleanup, GIMPLE_TRY_FINALLY);
> + seq = NULL;
> + gimplify_seq_add_stmt (&seq, gs);
> + }
> +
> outer_stmt = gimple_seq_first_stmt (seq);
> if (!outer_stmt)
> {
> diff --git a/gcc/testsuite/gcc.dg/guality/pr54200-2.c
> b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
> index e30e3c92b94..646790af65a 100644
> --- a/gcc/testsuite/gcc.dg/guality/pr54200-2.c
> +++ b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
> @@ -1,6 +1,5 @@
> /* { dg-do run } */
> -/* { dg-skip-if "" { *-*-* } { "*" } { "-Og" "-Os" "-O0" } } */
> -/* { dg-options "-g -fdebug-nops -DMAIN" } */
> +/* { dg-options "-g -fdebug-nops -fkeep-vars-live -DMAIN" } */
>
> #include "prevent-optimization.h"
>
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index 14d66b7a728..d3a4465fe25 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -4484,6 +4484,7 @@ verify_gimple_assign_single (gassign *stmt)
> case PARM_DECL:
> if (!is_gimple_reg (lhs)
> && !is_gimple_reg (rhs1)
> + && !TREE_CLOBBER_P (lhs)
> && is_gimple_reg_type (TREE_TYPE (lhs)))
> {
> error ("invalid rhs for gimple memory store");
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index 3e30f6bc3d4..f6488b90378 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -3536,6 +3536,13 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator
> *gsi)
> sra_stats.exprs++;
> }
>
> + if (modify_this_stmt && TREE_CLOBBER_P (gimple_assign_lhs (stmt)))
> + {
> + gimple_assign_set_rhs1 (stmt, rhs);
> + gimple_assign_set_lhs (stmt, build_clobber (TREE_TYPE (rhs)));
I think you could modify the type of the lhs in-place since
CONSTRUCTORs are not shared.
> + return SRA_AM_MODIFIED;
> + }
> +
> if (modify_this_stmt)
> {
> if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
> diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
> index 7b25778307f..03f378fa4b2 100644
> --- a/gcc/tree-ssa-alias.c
> +++ b/gcc/tree-ssa-alias.c
> @@ -1368,6 +1368,10 @@ refs_may_alias_p_1 (ao_ref *ref1, ao_ref *ref2, bool
> tbaa_p)
> poly_int64 max_size1 = -1, max_size2 = -1;
> bool var1_p, var2_p, ind1_p, ind2_p;
>
> + if ((ref1->ref && TREE_CLOBBER_P (ref1->ref))
> + || (ref2->ref && TREE_CLOBBER_P (ref2->ref)))
> + return false;
> +
I think it would be better to not arrive here but I assume the uses
are visible as stores with VDEFs in GIMPLE?
> gcc_checking_assert ((!ref1->ref
> || TREE_CODE (ref1->ref) == SSA_NAME
> || DECL_P (ref1->ref)
> diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
> index fd24f84fb14..d83579a09c5 100644
> --- a/gcc/tree-ssa-structalias.c
> +++ b/gcc/tree-ssa-structalias.c
> @@ -4883,7 +4883,8 @@ find_func_aliases (struct function *fn, gimple *origt)
> tree lhsop = gimple_assign_lhs (t);
> tree rhsop = (gimple_num_ops (t) == 2) ? gimple_assign_rhs1 (t) : NULL;
>
> - if (rhsop && TREE_CLOBBER_P (rhsop))
> + if ((rhsop && TREE_CLOBBER_P (rhsop))
> + || (lhsop && TREE_CLOBBER_P (lhsop)))
> /* Ignore clobbers, they don't actually store anything into
> the LHS. */
> ;
> diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
> index 8ccbc85970a..953db9ed02d 100644
> --- a/gcc/tree-ssa-uninit.c
> +++ b/gcc/tree-ssa-uninit.c
> @@ -132,6 +132,9 @@ warn_uninit (enum opt_code wc, tree t, tree expr, tree
> var,
> if (is_gimple_assign (context)
> && gimple_assign_rhs_code (context) == COMPLEX_EXPR)
> return;
> + if (gimple_assign_single_p (context)
> + && TREE_CLOBBER_P (gimple_assign_lhs (context)))
> + return;
> if (!has_undefined_value_p (t))
> return;
I see no handling of uses in DCE so I assume that stmts will be kept
live even though there are no other uses than the artificial USE?
Is that the intention? If so, -Og was explicitely supposed to
elide dead code as much as possible (via CCP + compare&jump folding),
because this not only shrinks code but decreases compile-time. For
the same reason -Og performs inlining.
So I really wonder what class of optimizations we lose with
-fkeep-vars-live. Can you look at testsuite results with
RUNTESTFLAGS="--target_board=unix/\{,-fkeep-vars-live\}"?
Thus compare results with/without -fkeep-vars-live?
Thanks,
Richard.
