On 6/9/21 7:48 AM, Richard Biener wrote:
On Tue, Jun 8, 2021 at 4:48 PM Andrew MacLeod <amacl...@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. 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.
