On Wed, Jul 25, 2018 at 1:41 PM Richard Biener <[email protected]> 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 <[email protected]> > > > > 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.
