[RFC,2/3,debug] Add fkeep-vars-live

On Tue, Jul 24, 2018 at 01:30:30PM +0200, Tom de Vries wrote:
> On 07/16/2018 05:10 PM, Tom de Vries wrote:
> > On 07/16/2018 03:50 PM, Richard Biener wrote:
> >> On Mon, 16 Jul 2018, Tom de Vries wrote:
> >>> Any comments?
> >>
> >> Interesting idea.  I wonder if that should be generalized
> >> to other places
> > 
> > I kept the option name general, to allow for that.
> > 
> > And indeed, this is a point-fix patch. I've been playing around with a
> > more generic patch that adds nops such that each is_stmt .loc is
> > associated with a unique insn, but that was embedded in an
> > fkeep-vars-live branch, so this patch is minimally addressing the first
> > problem I managed to reproduce on trunk.
> > 
> >> and how we can avoid compare-debug failures
> >> (var-tracking usually doesn't change code-generation).
> >>
> > 
> 
> I'll post this patch series (the current state of my fkeep-vars-live
> branch) in reply to this email:
> 
>      1  [debug] Add fdebug-nops
>      2  [debug] Add fkeep-vars-live
>      3  [debug] Add fdebug-nops and fkeep-vars-live to Og only
> 
> Bootstrapped and reg-tested on x86_64. ChangeLog entries and function
> header comments missing.
> 
> Comments welcome.
> 

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.

Another drawback is that the fake uses confuse the unitialized warning
analysis, so that is switched off for -fkeep-vars-live.

PR debug/78685

[debug] Add fkeep-vars-live

---
 gcc/cfgexpand.c                          |  9 +++++++
 gcc/common.opt                           |  4 +++
 gcc/function.c                           |  5 ++--
 gcc/gimplify.c                           | 44 +++++++++++++++++++++++++++-----
 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                    |  2 +-
 10 files changed, 70 insertions(+), 12 deletions(-)

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?

Is the -fkeep-vars-live option -fcompare-debug friendly?

> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -3533,6 +3533,13 @@ expand_clobber (tree lhs)
>      }
>  }
>  
> +static void
> +expand_use (tree rhs)
> +{
> +  rtx target = expand_expr (rhs, NULL_RTX, VOIDmode, EXPAND_NORMAL);
> +  emit_use (target);
> +}

Missing function comment.

> +fkeep-vars-live
> +Common Report Var(flag_keep_vars_live) Optimization
> +Add artificial uses of local vars at end of scope.

at the end of scope?
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1264,6 +1264,23 @@ asan_poison_variables (hash_set<tree> *variables, bool poison, gimple_seq *seq_p
>      }
>  }
>  
> +static gimple_seq
> +gimple_build_uses (tree vars)

Missing function comment.

> --- 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)) ||

|| should go on the next line.

> +      (ref2->ref && TREE_CLOBBER_P (ref2->ref)))
> +    return false;
> +
>    gcc_checking_assert ((!ref1->ref
>  			|| TREE_CODE (ref1->ref) == SSA_NAME
>  			|| DECL_P (ref1->ref)

> --- a/gcc/tree-ssa-uninit.c
> +++ b/gcc/tree-ssa-uninit.c
> @@ -2628,7 +2628,7 @@ warn_uninitialized_phi (gphi *phi, vec<gphi *> *worklist,
>  static bool
>  gate_warn_uninitialized (void)
>  {
> -  return warn_uninitialized || warn_maybe_uninitialized;
> +  return (warn_uninitialized || warn_maybe_uninitialized) && !flag_keep_vars_live;
>  }

See above.

	Jakub

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.


> Is the -fkeep-vars-live option -fcompare-debug friendly?
> 

I think so, there's no reference to debug flags or instructions.

>> --- a/gcc/cfgexpand.c
>> +++ b/gcc/cfgexpand.c
>> @@ -3533,6 +3533,13 @@ expand_clobber (tree lhs)
>>      }
>>  }
>>  
>> +static void
>> +expand_use (tree rhs)
>> +{
>> +  rtx target = expand_expr (rhs, NULL_RTX, VOIDmode, EXPAND_NORMAL);
>> +  emit_use (target);
>> +}
> 
> Missing function comment.
> 
>> +fkeep-vars-live
>> +Common Report Var(flag_keep_vars_live) Optimization
>> +Add artificial uses of local vars at end of scope.
> 
> at the end of scope?

Is this better?
+Add artificial use for each local variable at the end of the
declaration scope

Thanks,
- Tom

On Jul 24, 2018, Tom de Vries <tdevries@suse.de> wrote:

> 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.

I suggest also adding such uses before sets, so that variables aren't
regarded as dead and get optimized out in ranges between the end of a
live range and a subsequent assignment.

On Tue, Jul 24, 2018 at 04:11:11PM -0300, Alexandre Oliva wrote:
> On Jul 24, 2018, Tom de Vries <tdevries@suse.de> wrote:
> 
> > 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.
> 
> I suggest also adding such uses before sets, so that variables aren't
> regarded as dead and get optimized out in ranges between the end of a
> live range and a subsequent assignment.

But that can be done incrementally, right, and perhaps being controllable
by a level of this option, because such extra uses might make it even more
costly.  Though, if the extra uses and sets aren't in the same stmt, then
the optimizers could still move it appart (in addition of it making the IL
larger).
We need to think about different cases (non-gimple reg vars are probably ok,
they just live in memory unless converted (sra etc.) into gimple reg vars,
then what to do about SSA_NAMEs with underlying user variable if it doesn't
have overlapping ranges, if it does have overlapping ranges).

	Jakub

On Jul 25, 2018, Jakub Jelinek <jakub@redhat.com> wrote:

> On Tue, Jul 24, 2018 at 04:11:11PM -0300, Alexandre Oliva wrote:
>> On Jul 24, 2018, Tom de Vries <tdevries@suse.de> wrote:
>> 
>> > 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.
>> 
>> I suggest also adding such uses before sets, so that variables aren't
>> regarded as dead and get optimized out in ranges between the end of a
>> live range and a subsequent assignment.

> But that can be done incrementally, right

Sure

> the optimizers could still move it appart

*nod*