This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, debug] Add fkeep-vars-live


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]