[PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement.
Wed Jun 9 11:48:24 GMT 2021
On Tue, Jun 8, 2021 at 4:48 PM Andrew MacLeod <email@example.com> wrote:
> On 6/2/21 3:29 AM, Richard Biener wrote:
> > On Tue, Jun 1, 2021 at 4:24 PM Andrew MacLeod <firstname.lastname@example.org> wrote:
> >> On 6/1/21 3:34 AM, Richard Biener wrote:
> >>> On Tue, Jun 1, 2021 at 3:38 AM Andrew MacLeod via Gcc-patches
> >>> <email@example.com> wrote:
> >>>> An ongoing issue is the the order we evaluate things in can affect
> >>>> decisions along the way. As ranger isn't a fully iterative pass, we can
> >>>> sometimes come up with different results if back edges are processed in
> >>>> different orders.
> >>>> One of the ways this can happen is when the cache is propagating
> >>>> on-entry values for an SSA_NAME. It calculates outgoing edge values and
> >>>> the gori-compute engine can flag ssa-names that were involved in a range
> >>>> calculation that have not yet been initialized. When the propagation
> >>>> for the original name is done, it goes back and examines the "poor
> >>>> values" and tries to quickly calculate a better range, and if it comes
> >>>> up with one, immediately tries to go back and update the location/range
> >>>> gori_compute flagged. This produces better ranges earlier.
> >>>> However, when we do this in different orders, we can get different
> >>>> results. We were processing the uses on is_gimple_debug statements just
> >>>> like normal uses, and this would sometimes cause a difference in how
> >>>> things were resolved.
> >>>> This patch adds a flag to enable/disable this attempt to look up new
> >>>> values, and when range_of_expr is processing the use on a debug
> >>>> statement, turns it off for the query. This means the query will never
> >>>> cause a new lookup, and this should resolve all the -fcompare-debug issues.
> >>>> Bootstrapped on x86_64-pc-linux-gnu, with no new regressions. Pushed.
> >>> Please check if such fixes also apply to the GCC 11 branch.
> >>> Richard.
> >> I've checked both testcases against gcc11 release, and neither is an
> >> issue there. Much of this was triggered by changes to the export list.
> >> That said, is there potential for it to surface? The potential is
> >> probably there. We'd have to address it differently tho. For the
> >> gcc11 release, since we always run in hybrid mode it doesn't really
> >> matter if ranger looks up ranges for debug statements... EVRP will still
> >> pick up what we use to get for them. we could simply disable looking
> >> for contextual ranges for is_gimple_stmt and simply pick up the best
> >> known global/on-entry value available.. I can either provide a patch
> >> for that now, or deal with it if we ever get a PR. I'm ok either way.
> > I think it would be good to robustify the code even w/o a PR.
> >> btw, when is the next point release? I added an infrastructure patch to
> >> trunk (https://gcc.gnu.org/pipermail/gcc-patches/2021-May/569884.html)
> >> to enable replacing the on-entry cache to deal with memory consumption
> >> issues like in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100299 . I
> >> specifically put it in early before the other changes so that it could
> >> be directly applied to gcc11 as well, but I need to follow up with one
> >> of the replacements I have queued up to look at if we are interested in
> >> fixing this in gcc 11. I'll bump the priority to try to hit the next
> >> release if thats the case.
> > The first point release is usuall about two month from the initial release
> > which means in about a month and a half. It would be nice to fix
> > those issues and the earlier in the release series the better.
> > 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.
diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index 6158a754dd6..fd7fa5e3dbb 100644
@@ -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);
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
#0 fancy_abort (file=0x2a71420 "../../src/gcc-11-branch/gcc/gimple-range.cc",
tree_node*, gimple*)::__FUNCTION__> "range_of_expr")
#1 0x0000000001f28275 in gimple_ranger::range_of_expr (this=0x3274eb0, r=...,
expr=<ssa_name 0x7ffff66d3000 8>, stmt=<gimple_debug 0x7ffff657e300>)
#2 0x000000000151ab7c in range_query::value_of_expr (this=0x3274eb0,
name=<ssa_name 0x7ffff66d3000 8>, stmt=<gimple_debug 0x7ffff657e300>)
#3 0x0000000001f36ce3 in hybrid_folder::value_of_expr (this=0x7fffffffd990,
op=<ssa_name 0x7ffff66d3000 8>, stmt=<gimple_debug 0x7ffff657e300>)
#4 0x0000000001387804 in substitute_and_fold_engine::replace_uses_in (
this=0x7fffffffd990, stmt=<gimple_debug 0x7ffff657e300>)
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
phase to always only use global ranges? Maybe add the ability to
"lock" a ranger instance (disabling any further on-demand processing)?
Anyway, inheritance is a bit mazy here, so the patch does look sensible.
More information about the Gcc-patches