* [PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement. @ 2021-06-01 1:32 Andrew MacLeod 2021-06-01 7:34 ` Richard Biener 0 siblings, 1 reply; 8+ messages in thread From: Andrew MacLeod @ 2021-06-01 1:32 UTC (permalink / raw) To: gcc-patches [-- 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement. 2021-06-01 1:32 [PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement Andrew MacLeod @ 2021-06-01 7:34 ` Richard Biener 2021-06-01 14:23 ` Andrew MacLeod 0 siblings, 1 reply; 8+ messages in thread From: Richard Biener @ 2021-06-01 7:34 UTC (permalink / raw) To: Andrew MacLeod; +Cc: gcc-patches 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement. 2021-06-01 7:34 ` Richard Biener @ 2021-06-01 14:23 ` Andrew MacLeod 2021-06-02 7:29 ` Richard Biener 0 siblings, 1 reply; 8+ messages in thread From: Andrew MacLeod @ 2021-06-01 14:23 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement. 2021-06-01 14:23 ` Andrew MacLeod @ 2021-06-02 7:29 ` Richard Biener 2021-06-08 14:48 ` Andrew MacLeod 0 siblings, 1 reply; 8+ messages in thread From: Richard Biener @ 2021-06-02 7:29 UTC (permalink / raw) To: Andrew MacLeod; +Cc: gcc-patches 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement. 2021-06-02 7:29 ` Richard Biener @ 2021-06-08 14:48 ` Andrew MacLeod 2021-06-09 11:48 ` Richard Biener 0 siblings, 1 reply; 8+ messages in thread From: Andrew MacLeod @ 2021-06-08 14:48 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches, Jakub Jelinek [-- 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement. 2021-06-08 14:48 ` Andrew MacLeod @ 2021-06-09 11:48 ` Richard Biener 2021-06-09 15:24 ` Andrew MacLeod 0 siblings, 1 reply; 8+ messages in thread From: Richard Biener @ 2021-06-09 11:48 UTC (permalink / raw) To: Andrew MacLeod; +Cc: gcc-patches, Jakub Jelinek 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 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement. 2021-06-09 11:48 ` Richard Biener @ 2021-06-09 15:24 ` Andrew MacLeod 2021-06-10 5:59 ` Richard Biener 0 siblings, 1 reply; 8+ messages in thread From: Andrew MacLeod @ 2021-06-09 15:24 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches, Jakub Jelinek 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement. 2021-06-09 15:24 ` Andrew MacLeod @ 2021-06-10 5:59 ` Richard Biener 0 siblings, 0 replies; 8+ messages in thread From: Richard Biener @ 2021-06-10 5:59 UTC (permalink / raw) To: Andrew MacLeod; +Cc: gcc-patches, Jakub Jelinek 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-06-10 5:59 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-01 1:32 [PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement Andrew MacLeod 2021-06-01 7:34 ` Richard Biener 2021-06-01 14:23 ` Andrew MacLeod 2021-06-02 7:29 ` Richard Biener 2021-06-08 14:48 ` Andrew MacLeod 2021-06-09 11:48 ` Richard Biener 2021-06-09 15:24 ` Andrew MacLeod 2021-06-10 5:59 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).