Re: [PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement.

[Richard Biener <richard.guenther@gmail.com>]

On Wed, Jun 9, 2021 at 5:24 PM Andrew MacLeod <amacleod@redhat.com> wrote:
>
> On 6/9/21 7:48 AM, Richard Biener wrote:
> > On Tue, Jun 8, 2021 at 4:48 PM Andrew MacLeod <amacleod@redhat.com> wrote:
> >>
> >>
> >> Richard.
> >>
> >>>> Andrew
> >>>>
> >> OK, so this would be the simple way I'd tackle this in gcc11. This
> >> should be quite safe.  Just treat debug_stmts as if they are not stmts..
> >> and make a global query.   EVRP will still provide a contextual range as
> >> good as it ever did, but it wont trigger ranger lookups on debug uses
> >> any more.
> >>
> >> It bootstraps on x86_64-pc-linux-gnu.  Is there a process other than
> >> getting the OK to check this into the gcc 11 branch?  Does it go into
> >> releases/gcc-11 ?
> > it would go into releases/gcc-11, yes.
> >
> > Now,
> >
> > diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
> > index 6158a754dd6..fd7fa5e3dbb 100644
> > --- a/gcc/gimple-range.cc
> > +++ b/gcc/gimple-range.cc
> > @@ -945,7 +945,7 @@ gimple_ranger::range_of_expr (irange &r, tree
> > expr, gimple *stmt)
> >       return get_tree_range (r, expr);
> >
> >     // If there is no statement, just get the global value.
> > -  if (!stmt)
> > +  if (!stmt || is_gimple_debug (stmt))
> >       {
> >
> > unfortunately the function is not documented so I'm just guessing here - why
> > do we end up passing in a debug stmt as 'stmt'?  (how should expr and stmt
> > relate?)  So isn't it better to do this check before
> >
> >    if (!gimple_range_ssa_p (expr))
> >      return get_tree_range (r, expr);
> This parts just handles the non-ssa names, so constants, types , things
> for which there is no lookup involved.. At least in GCC 11.
> >
> > or even more better, assert we don't get a debug stmt here and fixup whoever
> > calls range_of_expr to not do that for debug stmts?  When I add this
> > assertion not even libgcc can configure...  backtraces look like
>
> range_of_expr is the basic API for asking for the range of EXPR as if it
> occurs as a use on STMT.  STMT provides the context for a location in
> the IL.  if STMT isn't provided, it picks up the global range. EXPR does
> not necessarily have to occur on stmt, it's just the context point for
> finding the range.   It should be documented in value-query.h where it
> is initially declared, but I see it is not.  Sorry about that.. It seems
> to have gotten lost in the myriad of moves that were made. We have a
> definite lack of documentation on everything... that is next in
> priority,  once I get the remaining relation code in.
>
> I don't think its wrong to supply a debug stmt.  stmt is simply the
> location in the IL for which we are querying the range of EXPR. So this
> is something like
>
> # DEBUG d => d_10
>
> and the query is asking for the range of d_10 at this point in the IL..
> ie, what would it be on this stmt.   There isn't anything wrong with
> that..  and we certainly make no attempt to stop it for that reason..
> This change does prevent any analytics from happening (as does the one
> on trunk).
>
> >
> > #0  fancy_abort (file=0x2a71420 "../../src/gcc-11-branch/gcc/gimple-range.cc",
> >      line=944,
> >      function=0x2a71638 <gimple_ranger::range_of_expr(irange&,
> > tree_node*, gimple*)::__FUNCTION__> "range_of_expr")
> >      at ../../src/gcc-11-branch/gcc/diagnostic.c:1884
> > #1  0x0000000001f28275 in gimple_ranger::range_of_expr (this=0x3274eb0, r=...,
> >      expr=<ssa_name 0x7ffff66d3000 8>, stmt=<gimple_debug 0x7ffff657e300>)
> >      at ../../src/gcc-11-branch/gcc/gimple-range.cc:944
> > #2  0x000000000151ab7c in range_query::value_of_expr (this=0x3274eb0,
> >      name=<ssa_name 0x7ffff66d3000 8>, stmt=<gimple_debug 0x7ffff657e300>)
> >      at ../../src/gcc-11-branch/gcc/value-query.cc:86
> > #3  0x0000000001f36ce3 in hybrid_folder::value_of_expr (this=0x7fffffffd990,
> >      op=<ssa_name 0x7ffff66d3000 8>, stmt=<gimple_debug 0x7ffff657e300>)
> >      at ../../src/gcc-11-branch/gcc/gimple-ssa-evrp.c:235
> > #4  0x0000000001387804 in substitute_and_fold_engine::replace_uses_in (
> >      this=0x7fffffffd990, stmt=<gimple_debug 0x7ffff657e300>)
> >      at ../../src/gcc-11-branch/gcc/tree-ssa-propagate.c:871
> >
> > so after EVRP we substitute and fold - but note we're not expecting to do
> > any more analysis in this phase but simply use the computed lattice,
> > since we don't substitute in unreachable code regions and thus SSA form
> > is temporarily broken that might otherwise cause issues.
> >
> > But yes, substitute and fold does substitute into debug stmts (but we don't
> > analyze debug stmts).  So maybe somehow arrange for the substitute_and_fold
> In which case this change is exactly what is needed. S&F will call
> range_of_expr asking for the range on the debug_stmt, and this change
> returns the global range instead of looking it up.
> > phase to always only use global ranges?  Maybe add the ability to
> > "lock" a ranger instance (disabling any further on-demand processing)?
>
> The change on trunk is better as it effectively makes debug stmts always
> use whatever the best value we know is without doing anything new. It
> only reverts to the global range if there is nothing better.  It depends
> on a bunch of other structural changes I wouldn't want to try to port
> back to gcc 11, too much churn.
>
> It might be possible to "lock" ranger, but the concept of a lattice
> doesn't really apply. There is no lattice..  there are only values as
> they appear at various points in the IL. We tracxk that mostly by
> propagating ranges to basic blocks.  When a query is made, if there is a
> readily available value it will use it. Unless it has reason to believe
> it is possible to improve it, in which case it may decide to go and check.
>
>
> > Anyway, inheritance is a bit mazy here, so the patch does look sensible.
> >
> So what this boils down to I think is that disabling any kind of lookup
> from a debug stmt is generally a good thing. That was a oversight on my
> part.  I think for GCC 11 this is sufficient. It will introduce no new
> analytics for debug stmts.
>
> For trunk, we could consider a lockdown, but I'm not sure even that is
> sufficient.. "When" do you lock it down..   The old model being used was
> "once a value is set, we don't revisit it", but we revisit things
> frequently during the initial DOM walk if the edges that need updating
> take us there.

So I was thinking of locking it down before the substitute & fold phase
given there is the analysis & propagation stage before.  Was there before,
not sure if it still is when ranger is used or whether everything is done
from the substitute-and-fold DOM walk.

>  When we get to the bottom of a loop, if we've learned
> something new, we propagate that back to the top and update what needs
> updating.  we can't lock that down, but it still changes the values that
> were originally seen thanks to the backend propagation.   Locking it
> down after the DOM walk will prevent anything new from happening then,
> but along the way things were done which cause issues. like the earlier
> case where we evolve to a constant, then further evolved to UNDEFINED.
>
>    I know the S&F mechanism is mostly driven by this "we decided it was
> a constant, substitute it everywhere" model  which is a bit orthogonal
> to how we operate... It has required a few concessions to map to that
> model since that isnt our "bottom" or "top", but I think we have a
> handle on them now.  Ideally when we get thru more of this, I'd like to
> change the way RVRP does the substitution too, as we've found it a bit
> limiting by times.
>
> If we run into more issues with the S&F engine, I'll see if disabling
> lookups and reverting to just a non-invasive query at the end resolves
> it better.
>
> I do think this is the right patch for GCC11, and it should be quite safe.

OK - agreed.  Thanks for the detailed explanation.

Richard.