[PATCH, debug] Add fkeep-vars-live
Richard Biener
richard.guenther@gmail.com
Wed Jul 25 11:47:00 GMT 2018
On Wed, Jul 25, 2018 at 1:41 PM Richard Biener <rguenther@suse.de> wrote:
>
> 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 <tdevries@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?
Btw, I really wonder if the RA could deal with an extra set of "weak" uses
that enlarge live ranges to a desired point but which doesn't have to be
fulfilled. I suppose "easiest" would be to split the live range at the
real point and add a completely artificial live-range with lower priority
that are only used by a debugger (thus spilling is prefered when no
hardreg is available). I realize infrastructure like DF comes into play here
but I'm more interested in this question from a RA theory perspective
since the RA already should be able to consider "cold" uses vs "hot"
ones?
Richard.
> Thanks,
> Richard.
More information about the Gcc-patches
mailing list