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

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

[Andrew MacLeod]

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]

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.

Andrew


[-- Attachment #2: 0004-Do-not-calculate-new-values-when-evaluating-a-debug-.patch --]
[-- Type: text/x-patch, Size: 4197 bytes --]

From 715914d3f9e4e40af58d22103c7650cdd720ef92 Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Mon, 31 May 2021 12:13:50 -0400
Subject: [PATCH 4/4] Do not calculate new values when evaluating a debug
 statement.

Add a flag to enable/disable immediately improving poor values found during
cache propagation. Then disable it when processing debug statements.

	gcc/
	PR tree-optimization/100781
	* gimple-range-cache.cc (ranger_cache::ranger_cache): Enable new
	value calculation by default.
	(ranger_cache::enable_new_values): New.
	(ranger_cache::disable_new_values): New.
	(ranger_cache::push_poor_value): Check if new values are allowed.
	* gimple-range-cache.h (class ranger_cache): New member/methods.
	* gimple-range.cc (gimple_ranger::range_of_expr): Check for debug
	statement, and disable/renable new value calculation.

	gcc/testsuite/
	PR tree-optimization/100781
	* gcc.dg/pr100781.c: New.
---
 gcc/gimple-range-cache.cc       | 20 ++++++++++++++++++++
 gcc/gimple-range-cache.h        |  3 +++
 gcc/gimple-range.cc             |  9 +++++++++
 gcc/testsuite/gcc.dg/pr100781.c | 25 +++++++++++++++++++++++++
 4 files changed, 57 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pr100781.c

diff --git a/gcc/gimple-range-cache.cc b/gcc/gimple-range-cache.cc
index dc32841310a..cc27574b7b4 100644
--- a/gcc/gimple-range-cache.cc
+++ b/gcc/gimple-range-cache.cc
@@ -586,6 +586,7 @@ ranger_cache::ranger_cache (gimple_ranger &q) : query (q)
       if (bb)
 	m_gori.exports (bb);
     }
+  enable_new_values ();
 }
 
 ranger_cache::~ranger_cache ()
@@ -606,6 +607,23 @@ ranger_cache::dump (FILE *f)
   fprintf (f, "\n");
 }
 
+// Allow the cache to flag and query new values when propagation is forced
+// to use an unknown value.
+
+void
+ranger_cache::enable_new_values ()
+{
+  m_new_value_p = true;
+}
+
+// Disable new value querying.
+
+void
+ranger_cache::disable_new_values ()
+{
+  m_new_value_p = false;
+}
+
 // Dump the caches for basic block BB to file F.
 
 void
@@ -689,6 +707,8 @@ ranger_cache::set_global_range (tree name, const irange &r)
 bool
 ranger_cache::push_poor_value (basic_block bb, tree name)
 {
+  if (!m_new_value_p)
+    return false;
   if (m_poor_value_list.length ())
     {
       // Don't push anything else to the same block.  If there are multiple 
diff --git a/gcc/gimple-range-cache.h b/gcc/gimple-range-cache.h
index fee69bcc578..4af461d2aa3 100644
--- a/gcc/gimple-range-cache.h
+++ b/gcc/gimple-range-cache.h
@@ -100,6 +100,8 @@ public:
   bool get_non_stale_global_range (irange &r, tree name);
   void set_global_range (tree name, const irange &r);
 
+  void enable_new_values ();
+  void disable_new_values ();
   non_null_ref m_non_null;
   gori_compute m_gori;
 
@@ -131,6 +133,7 @@ private:
   bool push_poor_value (basic_block bb, tree name);
   vec<update_record> m_poor_value_list;
   class gimple_ranger &query;
+  bool m_new_value_p;
 };
 
 #endif // GCC_SSA_RANGE_CACHE_H
diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index d58e151eb4e..ed0a0c9702b 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -971,6 +971,15 @@ gimple_ranger::range_of_expr (irange &r, tree expr, gimple *stmt)
       return true;
     }
 
+  // For a debug stmt, pick the best value currently available, do not
+  // trigger new value calculations.  PR 100781.
+  if (is_gimple_debug (stmt))
+    {
+      m_cache.disable_new_values ();
+      m_cache.range_of_expr (r, expr, stmt);
+      m_cache.enable_new_values ();
+      return true;
+    }
   basic_block bb = gimple_bb (stmt);
   gimple *def_stmt = SSA_NAME_DEF_STMT (expr);
 
diff --git a/gcc/testsuite/gcc.dg/pr100781.c b/gcc/testsuite/gcc.dg/pr100781.c
new file mode 100644
index 00000000000..c0e008a3ba5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr100781.c
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 --param=evrp-mode=ranger -fcompare-debug  " } */
+
+struct a {
+  int b;
+};
+long c(short d, long e, struct a f) {
+g:;
+  int h = f.b <= e, i = d, n = h >= d;
+  if (!n)
+    goto j;
+  goto k;
+j:;
+  long l = 5;
+  if (l)
+    goto m;
+  d = 0;
+m:
+  if (d)
+    return f.b;
+k:
+  goto g;
+}
+int main() { }
+
-- 
2.17.2

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

[Richard Biener]

On Tue, Jun 1, 2021 at 3:38 AM Andrew MacLeod via Gcc-patches
<gcc-patches@gcc.gnu.org> 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.

> Andrew
>

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

[Andrew MacLeod]

On 6/1/21 3:34 AM, Richard Biener wrote:
> On Tue, Jun 1, 2021 at 3:38 AM Andrew MacLeod via Gcc-patches
> <gcc-patches@gcc.gnu.org> 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.

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.

Andrew

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

[Richard Biener]

On Tue, Jun 1, 2021 at 4:24 PM Andrew MacLeod <amacleod@redhat.com> 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
> > <gcc-patches@gcc.gnu.org> 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
>

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

[Andrew MacLeod]

[-- Attachment #1: Type: text/plain, Size: 3971 bytes --]

On 6/2/21 3:29 AM, Richard Biener wrote:
> On Tue, Jun 1, 2021 at 4:24 PM Andrew MacLeod <amacleod@redhat.com> 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
>>> <gcc-patches@gcc.gnu.org> 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 ?

Andrew




[-- Attachment #2: 0004-Don-t-process-lookups-for-debug-statements-in-Ranger.patch --]
[-- Type: text/x-patch, Size: 1194 bytes --]

From ff5ab360b21a83ac84b1fff22d091df2c44dafdf Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Tue, 8 Jun 2021 09:43:17 -0400
Subject: [PATCH 4/4] Don't process lookups for debug statements in Ranger.

Although PR 100781 is not an issue in GCC11, its possible that a similar
situation may arise.  The identical fix cannot be easily introduced.
With EVRP always running in hybrid mode, there is no need for ranger to
spawn a lookup for a debug statement in this release.

	* gimple-range.cc (gimple_ranger::range_of_expr): Treat debug statments
	as contextless queries to avoid additional lookups.
---
 gcc/gimple-range.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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))
     {
       if (!m_cache.get_global_range (r, expr))
         r = gimple_range_global (expr);
-- 
2.25.4

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

[Richard Biener]

On Tue, Jun 8, 2021 at 4:48 PM Andrew MacLeod <amacleod@redhat.com> wrote:
>
> On 6/2/21 3:29 AM, Richard Biener wrote:
> > On Tue, Jun 1, 2021 at 4:24 PM Andrew MacLeod <amacleod@redhat.com> 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
> >>> <gcc-patches@gcc.gnu.org> 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.

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

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

Richard.

> Andrew
>
>
>

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

[Andrew MacLeod]

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

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

[Richard Biener]

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.