Re: Regression with "recomputation and PR 109154"
On 3/31/23 19:31, Hans-Peter Nilsson wrote: Date: Fri, 31 Mar 2023 15:48:22 -0400 From: Andrew MacLeod via Gcc-patches Reply-To: Andrew MacLeod commit 55bf4f0d443e5adbacfcdbbebf4b2e0c74d1dcc8 Author: Andrew MacLeod Date: Fri Mar 31 15:42:43 2023 -0400 Adjust testcases to not produce errors.. tree-optimization/109363 gcc/testsuite/ * g++.dg/warn/Wstringop-overflow-4.C: Always cehck bogus message. * gcc.dg/tree-ssa/pr23109.c: Disable better recomputations. (Needs to be spelled "PR tree-optimization/109363" for the bugzilla-marker hook to react. I'll mark manually though.) Oops! thanks :-) Andrew
Re: Regression with "recomputation and PR 109154"
> Date: Fri, 31 Mar 2023 15:48:22 -0400 > From: Andrew MacLeod via Gcc-patches > Reply-To: Andrew MacLeod > commit 55bf4f0d443e5adbacfcdbbebf4b2e0c74d1dcc8 > Author: Andrew MacLeod > Date: Fri Mar 31 15:42:43 2023 -0400 > > Adjust testcases to not produce errors.. > > tree-optimization/109363 > gcc/testsuite/ > * g++.dg/warn/Wstringop-overflow-4.C: Always cehck bogus message. > * gcc.dg/tree-ssa/pr23109.c: Disable better recomputations. (Needs to be spelled "PR tree-optimization/109363" for the bugzilla-marker hook to react. I'll mark manually though.) Thanks! I was about to push the --param adjustment as obvious, seeing consensus equivalent of approval in this mail thread. brgds, H-P
Re: Regression with "recomputation and PR 109154"
On 3/31/23 14:16, Andrew MacLeod wrote: On 3/31/23 15:59, Jeff Law wrote: On 3/31/23 13:48, Andrew MacLeod wrote: should we do something like this to tweak the testcases? or does someone have something else in mind? Go ahead and tweak the testcase. Unless you want to revamp it per Jakub's suggestions. not particularly :-) pushed. hopefully this smooths things a little... probably causes them to fail elsewhere now. ha! Im out for most of the rest of the weekend.. any other tweaks someone else should do... I will check in once in a while. If that causes the test to fail elsewhere, we'll know by Monday. jeff
Re: Regression with "recomputation and PR 109154"
On 3/31/23 15:59, Jeff Law wrote: On 3/31/23 13:48, Andrew MacLeod wrote: should we do something like this to tweak the testcases? or does someone have something else in mind? Go ahead and tweak the testcase. Unless you want to revamp it per Jakub's suggestions. not particularly :-) pushed. hopefully this smooths things a little... probably causes them to fail elsewhere now. ha! Im out for most of the rest of the weekend.. any other tweaks someone else should do... I will check in once in a while. Andrew
Re: Regression with "recomputation and PR 109154"
On 3/31/23 13:48, Andrew MacLeod wrote: should we do something like this to tweak the testcases? or does someone have something else in mind? Go ahead and tweak the testcase. Unless you want to revamp it per Jakub's suggestions. jeff
Re: Regression with "recomputation and PR 109154"
should we do something like this to tweak the testcases? or does
someone have something else in mind?
Richi opened a PR for the STL failure (109350)
Andrew
On 3/31/23 13:37, Jakub Jelinek wrote:
On Fri, Mar 31, 2023 at 01:02:18PM -0400, Andrew MacLeod wrote:
I guess it figures the recip is safe to put in, there will not be a divide
by zero.
I think the problem was that 1/d was hoisted before the loop; as long as
it is guarded with the d > 0.01 or e > 0.005 condition, it is fine.
The test probably should have been a runtime test, doing the main stuff
in some other noipa function and doing fetestexcept after it or something
similar.
I guess the test is no longer testing what it should be?
And yes, we could set he param back to 1 for the test...
add --param=ranger-recompute-depth=1 makes the "issue" go away :-) for
now.
That looks reasonable unless we rewrite the test into runtime one (but we'd
then need to double check that it was really miscompiled and would fail back
then in 4.0).
Jakub
commit 55bf4f0d443e5adbacfcdbbebf4b2e0c74d1dcc8
Author: Andrew MacLeod
Date: Fri Mar 31 15:42:43 2023 -0400
Adjust testcases to not produce errors..
tree-optimization/109363
gcc/testsuite/
* g++.dg/warn/Wstringop-overflow-4.C: Always cehck bogus message.
* gcc.dg/tree-ssa/pr23109.c: Disable better recomputations.
diff --git a/gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C b/gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C
index 35fb59e0232..faad5bed074 100644
--- a/gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C
+++ b/gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C
@@ -141,7 +141,7 @@ void test_strcpy_new_int16_t (size_t n, const size_t vals[])
int r_imin_imax = SR (INT_MIN, INT_MAX);
T (S (1), new int16_t[r_imin_imax]);
- T (S (2), new int16_t[r_imin_imax + 1]); // { dg-bogus "into a region of size" "pr106120" { xfail { ilp32 && c++98_only } } }
+ T (S (2), new int16_t[r_imin_imax + 1]); // { dg-bogus "into a region of size" "pr106120" { xfail { c++98_only } } }
T (S (9), new int16_t[r_imin_imax * 2 + 1]);
int r_0_imax = SR (0, INT_MAX);
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr23109.c b/gcc/testsuite/gcc.dg/tree-ssa/pr23109.c
index 7cdf1d05ee7..059f658ea20 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr23109.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr23109.c
@@ -1,6 +1,7 @@
/* { dg-do compile } */
-/* { dg-options "-O2 -funsafe-math-optimizations -ftrapping-math -fdump-tree-recip -fdump-tree-lim2" } */
+/* { dg-options "-O2 -funsafe-math-optimizations -ftrapping-math -fdump-tree-recip -fdump-tree-lim2 --param=ranger-recompute-depth=1" } */
/* { dg-warning "'-fassociative-math' disabled" "" { target *-*-* } 0 } */
+/* ranger-recompute-depth prevents the optimizers from being too smart. */
double F[2] = { 0., 0. }, e = 0.;
Re: Regression with "recomputation and PR 109154"
On Fri, Mar 31, 2023 at 01:02:18PM -0400, Andrew MacLeod wrote: > I guess it figures the recip is safe to put in, there will not be a divide > by zero. I think the problem was that 1/d was hoisted before the loop; as long as it is guarded with the d > 0.01 or e > 0.005 condition, it is fine. The test probably should have been a runtime test, doing the main stuff in some other noipa function and doing fetestexcept after it or something similar. > I guess the test is no longer testing what it should be? > > And yes, we could set he param back to 1 for the test... > add --param=ranger-recompute-depth=1 makes the "issue" go away :-) for > now. That looks reasonable unless we rewrite the test into runtime one (but we'd then need to double check that it was really miscompiled and would fail back then in 4.0). Jakub
Re: Regression with "recomputation and PR 109154"
On 3/31/23 12:20, Jeff Law wrote: On 3/31/23 10:12, Hans-Peter Nilsson via Gcc-patches wrote: Attached. I also removed the bogus warning in Walloc-13.c that no longer happens Add recursive GORI recompuations with a depth limit. PR tree-optimization/109154 gcc/ * gimple-range-gori.cc (gori_compute::may_recompute_p): Add depth limit. * gimple-range-gori.h (may_recompute_p): Add depth param. * params.opt (ranger-recompute-depth): New param. gcc/testsuite/ * gcc.dg/Walloca-13.c: Remove bogus warning that is now fixed. This patch, commit r13-6945-g429a7a88438cc8, caused a test-suite regression; FAIL for 'gcc.dg/tree-ssa/pr23109.c scan-tree-dump-not recip "reciptmp"' for cris-elf that I logged at PR109363. Perhaps it's somewhat of a similar nature as Walloca-13.c but then again it's not an bogus warning that's gone. Is it expected and I should just blankly xfail it or is it worth more attention? I'm a bit surprised that it hasn't shown up for other targets though. I already let Andrew know -- it's affecting a ton of targets. It may be the case that we just need to adjust the param for that test. So the test fails in the testuite because a recip has been put into the code, but does it execute OK? or maybe thats not testable? with the recompuation, we make some signficant changes: e.0_1 = e; d_12 = e.0_1 * 2.0e+0; E_13 = 1.0e+0 - d_12; goto ; [INV] : if (e.0_1 > 5.00010408340855860842566471546888351440429688e-3) goto ; [INV] else goto ; [INV] : if (E_13 > 1.0e+0) goto ; [INV] else goto ; [INV] In EVRP we now determine that the branch in BB4 is never true and we reduce the testcase significantly We end up now with: [local count: 357878152]: e.0_1 = e; if (e.0_1 > 5.00010408340855860842566471546888351440429688e-3) goto ; [50.00%] else goto ; [50.00%] [local count: 178939076]: d_9 = e.0_1 * 2.0e+0; E_10 = 1.0e+0 - d_9; _16 = E_10 - 1.0e+0; reciptmp.5_21 = 1.0e+0 / d_9; And with d_9 : [frange] double [1.0001942890293094023945741355419158935546875e-2 (0x0.a3d70a3d70a3ep-6), +Inf] I guess it figures the recip is safe to put in, there will not be a divide by zero. I guess the test is no longer testing what it should be? And yes, we could set he param back to 1 for the test... add --param=ranger-recompute-depth=1 makes the "issue" go away :-) for now. Andrew
Re: Regression with "recomputation and PR 109154"
On 3/31/23 10:12, Hans-Peter Nilsson via Gcc-patches wrote: Attached. I also removed the bogus warning in Walloc-13.c that no longer happens Add recursive GORI recompuations with a depth limit. PR tree-optimization/109154 gcc/ * gimple-range-gori.cc (gori_compute::may_recompute_p): Add depth limit. * gimple-range-gori.h (may_recompute_p): Add depth param. * params.opt (ranger-recompute-depth): New param. gcc/testsuite/ * gcc.dg/Walloca-13.c: Remove bogus warning that is now fixed. This patch, commit r13-6945-g429a7a88438cc8, caused a test-suite regression; FAIL for 'gcc.dg/tree-ssa/pr23109.c scan-tree-dump-not recip "reciptmp"' for cris-elf that I logged at PR109363. Perhaps it's somewhat of a similar nature as Walloca-13.c but then again it's not an bogus warning that's gone. Is it expected and I should just blankly xfail it or is it worth more attention? I'm a bit surprised that it hasn't shown up for other targets though. I already let Andrew know -- it's affecting a ton of targets. It may be the case that we just need to adjust the param for that test. jeff
Regression with "recomputation and PR 109154"
> Attached. I also removed the bogus warning in Walloc-13.c that no longer > happens > Add recursive GORI recompuations with a depth limit. > > PR tree-optimization/109154 > gcc/ > * gimple-range-gori.cc (gori_compute::may_recompute_p): Add depth > limit. > * gimple-range-gori.h (may_recompute_p): Add depth param. > * params.opt (ranger-recompute-depth): New param. > > gcc/testsuite/ > * gcc.dg/Walloca-13.c: Remove bogus warning that is now fixed. This patch, commit r13-6945-g429a7a88438cc8, caused a test-suite regression; FAIL for 'gcc.dg/tree-ssa/pr23109.c scan-tree-dump-not recip "reciptmp"' for cris-elf that I logged at PR109363. Perhaps it's somewhat of a similar nature as Walloca-13.c but then again it's not an bogus warning that's gone. Is it expected and I should just blankly xfail it or is it worth more attention? I'm a bit surprised that it hasn't shown up for other targets though. brgds, H-P
Re: recomputation and PR 109154
On Thu, Mar 30, 2023 at 1:40 PM Andrew MacLeod via Gcc-patches wrote: > > I committed it. ran it again for fun. sigh. Looks like its also > triggering another issue now in g++.dg/warn/Wstringop-overflow-4.C > where its issuing: libstdc++v3's 23_containers/vector/bool/allocator/copy.cc fails most likely in a similar way. In file included from /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/algorithm:60,^M from /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/stdc++.h:51,^M from :^M In static member function 'static _Up* std::__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m(_Tp*, _Tp*, _Up*) [with _Tp = long unsigned int; _Up = long unsigned int; bool _IsMove = false]',^M inlined from '_OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = false; _II = long unsigned int*; _OI = long unsigned int*]' at /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:506,^M inlined from '_OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = false; _II = long unsigned int*; _OI = long unsigned int*]' at /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:533,^M inlined from '_OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = false; _II = long unsigned int*; _OI = long unsigned int*]' at /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:540,^M inlined from '_OI std::copy(_II, _II, _OI) [with _II = long unsigned int*; _OI = long unsigned int*]' at /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:633,^M inlined from 'std::vector::iterator std::vector::_M_copy_aligned(const_iterator, const_iterator, iterator) [with _Alloc = __gnu_test::propagating_allocator]' at /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_bvector.h:1303,^M inlined from 'void std::vector::_M_insert_aux(iterator, bool) [with _Alloc = __gnu_test::propagating_allocator]' at /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/vector.tcc:945,^M inlined from 'void std::vector::push_back(bool) [with _Alloc = __gnu_test::propagating_allocator]' at /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_bvector.h:1121,^M inlined from 'void test01()' at /home/apinski/src/upstream-gcc/gcc/libstdc++-v3/testsuite/23_containers/vector/bool/allocator/copy.cc:33:^M /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:437: warning: 'void* __builtin_memmove(void*, const void*, long unsigned int)' writing between 9 and 9223372036854775807 bytes into a region of size 8 overflows the destination [-Wstringop-overflow=]^M In file included from /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/c++allocator.h:33,^M from /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/allocator.h:46,^M from /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/string:43,^M from /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bitset:52,^M from /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/stdc++.h:52:^M In member function '_Tp* std::__new_allocator<_Tp>::allocate(size_type, const void*) [with _Tp = long unsigned int]',^M inlined from 'static _Tp* std::allocator_traits >::allocate(allocator_type&, size_type) [with _Tp = long unsigned int]' at /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/alloc_traits.h:482,^M inlined from '__gnu_test::uneq_allocator::pointer __gnu_test::uneq_allocator::allocate(size_type, const void*) [with Tp = long unsigned int; Alloc = std::allocator]' at /home/apinski/src/upstream-gcc/gcc/libstdc++-v3/testsuite/util/testsuite_allocator.h:360,^M inlined from 'static std::allocator_traits< >::pointer std::allocator_traits< >::allocate(_Alloc&, size_type) [with _Alloc = __gnu_test::propagating_allocator >]' at /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/alloc_traits.h:333,^M inlined from 'std::_Bvector_base<_Alloc>::_Bit_pointer std::_Bvector_base<_Alloc>::_M_allocate(std::size_t) [with _Alloc = __gnu_test::propagating_allocator]' at /home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_bvector.h:643,^M inlined from 'void std::vector::_M_insert_aux(iterator, bool) [with _Alloc = __gnu_test::propagating_allocator]' at
Re: recomputation and PR 109154
I committed it. ran it again for fun. sigh. Looks like its also
triggering another issue now in g++.dg/warn/Wstringop-overflow-4.C
where its issuing:
b.C: In function ‘void test_strcpy_new_int16_t(size_t, const size_t*)’:
b.C:76:12: warning: ‘void* __builtin_memcpy(void*, const void*, long
unsigned int)’ writing 3 bytes into a region of size 0
[-Wstringop-overflow=]
76 | strcpy (d, s); \
| ~~~^~
b.C:90:3: note: in expansion of macro ‘T’
90 | T (S (2), new int16_t[r_imin_imax + 1]); // { dg-bogus "into
a region of size" "pr106120" { xfail { ilp32 && c++98_only } } }
previously, by VRP2 time we hadn't figured out the edge case, couldn't
recalculate _29 and iftmp.1_38 was unknown.
iftmp.1_38 = _29 * 2;
_40 = operator new [] (iftmp.1_38);
__builtin_memcpy (_40, [(void
*)"0123456789abcdefghijklmnopqrstuvwxyz" + 34B], 3);
Now, by VRP2 we have figured it out...
_40 = operator new [] (0);
__builtin_memcpy (_40, [(void
*)"0123456789abcdefghijklmnopqrstuvwxyz" + 34B], 3);
And that info is available earlier for the warnings, just hasn't been
explcitly exposed:
Do we want to change the cfail to always? something like:
diff --git a/gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C
b/gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C
index 35fb59e0232..faad5bed074 100644
--- a/gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C
+++ b/gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C
@@ -141,7 +141,7 @@ void test_strcpy_new_int16_t (size_t n, const size_t
vals[])
int r_imin_imax = SR (INT_MIN, INT_MAX);
T (S (1), new int16_t[r_imin_imax]);
- T (S (2), new int16_t[r_imin_imax + 1]); // { dg-bogus "into a region
of size" "pr106120" { xfail { ilp32 && c++98_only } } }
+ T (S (2), new int16_t[r_imin_imax + 1]); // { dg-bogus "into a region
of size" "pr106120" { xfail { c++98_only } } }
T (S (9), new int16_t[r_imin_imax * 2 + 1]);
int r_0_imax = SR (0, INT_MAX);
Of course, I dont know what this is doing on other arches... perhaps
wait for the fallout to be complete?
Andrew
On 3/30/23 12:05, Jakub Jelinek wrote:
On Thu, Mar 30, 2023 at 11:58:19AM -0400, Andrew MacLeod wrote:
On 3/30/23 09:41, Jakub Jelinek wrote:
On Wed, Mar 29, 2023 at 01:22:27PM -0400, Andrew MacLeod wrote:
however, as seems to be the case often, better ranges result in, I now get:
FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)
Our middle-end warnings are just badly designed :(, the better value ranges
are, the more false positives they have.
commit 358d0ca44faf2e20fbacd0f74386308b5ca52cd4
Author: Andrew MacLeod
Date: Tue Mar 28 12:16:34 2023 -0400
Add recursive GORI recompuations with a depth limit.
LGTM for trunk, let's do with the regression incrementally.
Or as Richard mentioned on IRC, one possibility would be to force this
param temporarily to 1 (or whatever matches previous behavior) for the
diagnostic range queries).
You need a ChangeLog entry though...
Attached. I also removed the bogus warning in Walloc-13.c that no longer
happens
So incrementally deal with it.. what? just let it fail?
For today? Yes.
Ok for trunk.
commit debb8ce1f9b9d5a72d88d0ae90a6b4da5130ff59
Author: Andrew MacLeod
Date: Tue Mar 28 12:16:34 2023 -0400
Add recursive GORI recompuations with a depth limit.
PR tree-optimization/109154
gcc/
* gimple-range-gori.cc (gori_compute::may_recompute_p): Add depth
limit.
* gimple-range-gori.h (may_recompute_p): Add depth param.
* params.opt (ranger-recompute-depth): New param.
gcc/testsuite/
* gcc.dg/Walloca-13.c: Remove bogus warning that is now fixed.
Jakub
Re: recomputation and PR 109154
On Thu, Mar 30, 2023 at 11:58:19AM -0400, Andrew MacLeod wrote: > > On 3/30/23 09:41, Jakub Jelinek wrote: > > On Wed, Mar 29, 2023 at 01:22:27PM -0400, Andrew MacLeod wrote: > > > however, as seems to be the case often, better ranges result in, I now > > > get: > > > > > > FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors) > > Our middle-end warnings are just badly designed :(, the better value ranges > > are, the more false positives they have. > > > > > commit 358d0ca44faf2e20fbacd0f74386308b5ca52cd4 > > > Author: Andrew MacLeod > > > Date: Tue Mar 28 12:16:34 2023 -0400 > > > > > > Add recursive GORI recompuations with a depth limit. > > LGTM for trunk, let's do with the regression incrementally. > > Or as Richard mentioned on IRC, one possibility would be to force this > > param temporarily to 1 (or whatever matches previous behavior) for the > > diagnostic range queries). > > > > You need a ChangeLog entry though... > > > Attached. I also removed the bogus warning in Walloc-13.c that no longer > happens > > So incrementally deal with it.. what? just let it fail? For today? Yes. Ok for trunk. > commit debb8ce1f9b9d5a72d88d0ae90a6b4da5130ff59 > Author: Andrew MacLeod > Date: Tue Mar 28 12:16:34 2023 -0400 > > Add recursive GORI recompuations with a depth limit. > > PR tree-optimization/109154 > gcc/ > * gimple-range-gori.cc (gori_compute::may_recompute_p): Add depth > limit. > * gimple-range-gori.h (may_recompute_p): Add depth param. > * params.opt (ranger-recompute-depth): New param. > > gcc/testsuite/ > * gcc.dg/Walloca-13.c: Remove bogus warning that is now fixed. Jakub
Re: recomputation and PR 109154
On 3/30/23 09:41, Jakub Jelinek wrote:
On Wed, Mar 29, 2023 at 01:22:27PM -0400, Andrew MacLeod wrote:
however, as seems to be the case often, better ranges result in, I now get:
FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)
Our middle-end warnings are just badly designed :(, the better value ranges
are, the more false positives they have.
commit 358d0ca44faf2e20fbacd0f74386308b5ca52cd4
Author: Andrew MacLeod
Date: Tue Mar 28 12:16:34 2023 -0400
Add recursive GORI recompuations with a depth limit.
LGTM for trunk, let's do with the regression incrementally.
Or as Richard mentioned on IRC, one possibility would be to force this
param temporarily to 1 (or whatever matches previous behavior) for the
diagnostic range queries).
You need a ChangeLog entry though...
Attached. I also removed the bogus warning in Walloc-13.c that no longer
happens
So incrementally deal with it.. what? just let it fail?
Andrew
commit debb8ce1f9b9d5a72d88d0ae90a6b4da5130ff59
Author: Andrew MacLeod
Date: Tue Mar 28 12:16:34 2023 -0400
Add recursive GORI recompuations with a depth limit.
PR tree-optimization/109154
gcc/
* gimple-range-gori.cc (gori_compute::may_recompute_p): Add depth limit.
* gimple-range-gori.h (may_recompute_p): Add depth param.
* params.opt (ranger-recompute-depth): New param.
gcc/testsuite/
* gcc.dg/Walloca-13.c: Remove bogus warning that is now fixed.
diff --git a/gcc/gimple-range-gori.cc b/gcc/gimple-range-gori.cc
index 6e8dfa85ca8..5f4313b27dd 100644
--- a/gcc/gimple-range-gori.cc
+++ b/gcc/gimple-range-gori.cc
@@ -1308,7 +1308,7 @@ gori_compute::compute_operand1_and_operand2_range (vrange ,
// direct dependent is exported, it may also change the computed value of NAME.
bool
-gori_compute::may_recompute_p (tree name, basic_block bb)
+gori_compute::may_recompute_p (tree name, basic_block bb, int depth)
{
tree dep1 = depend1 (name);
tree dep2 = depend2 (name);
@@ -1322,22 +1322,36 @@ gori_compute::may_recompute_p (tree name, basic_block bb)
if (is_a (s) || gimple_has_side_effects (s))
return false;
- // If edge is specified, check if NAME can be recalculated on that edge.
- if (bb)
-return ((is_export_p (dep1, bb))
- || (dep2 && is_export_p (dep2, bb)));
+ if (!dep2)
+{
+ // -1 indicates a default param, convert it to the real default.
+ if (depth == -1)
+ {
+ depth = (int)param_ranger_recompute_depth;
+ gcc_checking_assert (depth >= 1);
+ }
- return (is_export_p (dep1)) || (dep2 && is_export_p (dep2));
+ bool res = (bb ? is_export_p (dep1, bb) : is_export_p (dep1));
+ if (res || depth <= 1)
+ return res;
+ // Check another level of recomputation.
+ return may_recompute_p (dep1, bb, --depth);
+}
+ // Two dependencies terminate the depth of the search.
+ if (bb)
+return is_export_p (dep1, bb) || is_export_p (dep2, bb);
+ else
+return is_export_p (dep1) || is_export_p (dep2);
}
// Return TRUE if NAME can be recomputed on edge E. If any direct dependent
// is exported on edge E, it may change the computed value of NAME.
bool
-gori_compute::may_recompute_p (tree name, edge e)
+gori_compute::may_recompute_p (tree name, edge e, int depth)
{
gcc_checking_assert (e);
- return may_recompute_p (name, e->src);
+ return may_recompute_p (name, e->src, depth);
}
diff --git a/gcc/gimple-range-gori.h b/gcc/gimple-range-gori.h
index 0fc90ec8a18..3ea4b45595b 100644
--- a/gcc/gimple-range-gori.h
+++ b/gcc/gimple-range-gori.h
@@ -172,8 +172,8 @@ private:
bool refine_using_relation (tree op1, vrange _range,
tree op2, vrange _range,
fur_source , relation_kind k);
- bool may_recompute_p (tree name, edge e);
- bool may_recompute_p (tree name, basic_block bb = NULL);
+ bool may_recompute_p (tree name, edge e, int depth = -1);
+ bool may_recompute_p (tree name, basic_block bb = NULL, int depth = -1);
bool compute_operand_range_switch (vrange , gswitch *s, const vrange ,
tree name, fur_source );
bool compute_operand1_range (vrange , gimple_range_op_handler ,
diff --git a/gcc/params.opt b/gcc/params.opt
index 2329d150ef0..b2ec436546c 100644
--- a/gcc/params.opt
+++ b/gcc/params.opt
@@ -900,6 +900,11 @@ Common Joined UInteger Var(param_ranger_logical_depth) Init(6) IntegerRange(1, 9
Maximum depth of logical expression evaluation ranger will look through when
evaluating outgoing edge ranges.
+-param=ranger-recompute-depth=
+Common Joined UInteger Var(param_ranger_recompute_depth) Init(5) IntegerRange(1, 100) Param Optimization
+Maximum depth of instruction chains to consider for recomputation in the
+outgoing range calculator.
+
-param=relation-block-limit=
Common Joined UInteger Var(param_relation_block_limit) Init(200) IntegerRange(0, ) Param Optimization
Maximum number of relations the oracle will register in a basic block.
Re: recomputation and PR 109154
On Wed, Mar 29, 2023 at 01:22:27PM -0400, Andrew MacLeod wrote:
> however, as seems to be the case often, better ranges result in, I now get:
>
> FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)
Our middle-end warnings are just badly designed :(, the better value ranges
are, the more false positives they have.
> commit 358d0ca44faf2e20fbacd0f74386308b5ca52cd4
> Author: Andrew MacLeod
> Date: Tue Mar 28 12:16:34 2023 -0400
>
> Add recursive GORI recompuations with a depth limit.
LGTM for trunk, let's do with the regression incrementally.
Or as Richard mentioned on IRC, one possibility would be to force this
param temporarily to 1 (or whatever matches previous behavior) for the
diagnostic range queries).
You need a ChangeLog entry though...
> diff --git a/gcc/gimple-range-gori.cc b/gcc/gimple-range-gori.cc
> index 6e8dfa85ca8..5f4313b27dd 100644
> --- a/gcc/gimple-range-gori.cc
> +++ b/gcc/gimple-range-gori.cc
> @@ -1308,7 +1308,7 @@ gori_compute::compute_operand1_and_operand2_range
> (vrange ,
> // direct dependent is exported, it may also change the computed value of
> NAME.
>
> bool
> -gori_compute::may_recompute_p (tree name, basic_block bb)
> +gori_compute::may_recompute_p (tree name, basic_block bb, int depth)
> {
>tree dep1 = depend1 (name);
>tree dep2 = depend2 (name);
> @@ -1322,22 +1322,36 @@ gori_compute::may_recompute_p (tree name, basic_block
> bb)
>if (is_a (s) || gimple_has_side_effects (s))
> return false;
>
> - // If edge is specified, check if NAME can be recalculated on that edge.
> - if (bb)
> -return ((is_export_p (dep1, bb))
> - || (dep2 && is_export_p (dep2, bb)));
> + if (!dep2)
> +{
> + // -1 indicates a default param, convert it to the real default.
> + if (depth == -1)
> + {
> + depth = (int)param_ranger_recompute_depth;
> + gcc_checking_assert (depth >= 1);
> + }
>
> - return (is_export_p (dep1)) || (dep2 && is_export_p (dep2));
> + bool res = (bb ? is_export_p (dep1, bb) : is_export_p (dep1));
> + if (res || depth <= 1)
> + return res;
> + // Check another level of recomputation.
> + return may_recompute_p (dep1, bb, --depth);
> +}
> + // Two dependencies terminate the depth of the search.
> + if (bb)
> +return is_export_p (dep1, bb) || is_export_p (dep2, bb);
> + else
> +return is_export_p (dep1) || is_export_p (dep2);
> }
>
> // Return TRUE if NAME can be recomputed on edge E. If any direct dependent
> // is exported on edge E, it may change the computed value of NAME.
>
> bool
> -gori_compute::may_recompute_p (tree name, edge e)
> +gori_compute::may_recompute_p (tree name, edge e, int depth)
> {
>gcc_checking_assert (e);
> - return may_recompute_p (name, e->src);
> + return may_recompute_p (name, e->src, depth);
> }
>
>
> diff --git a/gcc/gimple-range-gori.h b/gcc/gimple-range-gori.h
> index 0fc90ec8a18..3ea4b45595b 100644
> --- a/gcc/gimple-range-gori.h
> +++ b/gcc/gimple-range-gori.h
> @@ -172,8 +172,8 @@ private:
>bool refine_using_relation (tree op1, vrange _range,
> tree op2, vrange _range,
> fur_source , relation_kind k);
> - bool may_recompute_p (tree name, edge e);
> - bool may_recompute_p (tree name, basic_block bb = NULL);
> + bool may_recompute_p (tree name, edge e, int depth = -1);
> + bool may_recompute_p (tree name, basic_block bb = NULL, int depth = -1);
>bool compute_operand_range_switch (vrange , gswitch *s, const vrange
> ,
>tree name, fur_source );
>bool compute_operand1_range (vrange , gimple_range_op_handler ,
> diff --git a/gcc/params.opt b/gcc/params.opt
> index 2329d150ef0..b2ec436546c 100644
> --- a/gcc/params.opt
> +++ b/gcc/params.opt
> @@ -900,6 +900,11 @@ Common Joined UInteger Var(param_ranger_logical_depth)
> Init(6) IntegerRange(1, 9
> Maximum depth of logical expression evaluation ranger will look through when
> evaluating outgoing edge ranges.
>
> +-param=ranger-recompute-depth=
> +Common Joined UInteger Var(param_ranger_recompute_depth) Init(5)
> IntegerRange(1, 100) Param Optimization
> +Maximum depth of instruction chains to consider for recomputation in the
> +outgoing range calculator.
> +
> -param=relation-block-limit=
> Common Joined UInteger Var(param_relation_block_limit) Init(200)
> IntegerRange(0, ) Param Optimization
> Maximum number of relations the oracle will register in a basic block.
Jakub
Re: recomputation and PR 109154
On Wed, Mar 29, 2023 at 7:22 PM Andrew MacLeod wrote:
>
> The patch, or a slight variation (attached), in the PR allows us to
> generate better ranges be recomputing longer instruction sequences on
> outgoing edges.
>
> This in fact also fixes
> XPASS: gcc.dg/Walloca-13.c (test for bogus messages, line 11)
>
> [local count: 1073741824]:
>_1 = p_5(D) - q_6(D);
>_2 = _1 /[ex] 4;
>n_7 = (long unsigned int) _2;
>_11 = (long unsigned int) _1;
>if (_11 <= 396)
> goto ; [33.00%]
>else
> goto ; [67.00%]
>
> [local count: 354334800]:
>_3 = __builtin_alloca (n_7);
>
> Where _2 was recomputed before, but n_7 was not. Now it is, and we
> correctly do not issue the warning any more. awesome.,
>
> however, as seems to be the case often, better ranges result in, I now get:
>
> FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)
>
> because we now generate:
>
> /opt/notnfs/amacleod/master/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:437:
> warning: ‘void* __builtin_memmove(void*, const void*, long unsigned
> int)’ writing between 9 and 9223372036854775807 bytes into a region of
> size 8 overflows the destination [-Wstringop-overflow=]
>
> I see:
>
>
> _216 = operator new (8);
>
> _216 : [irange] long unsigned int * [1, +INF]
>..
>
> [local count: 86938296]:
> D.245552 ={v} {CLOBBER(eol)};
> _74 = v1.D.217578._M_impl.D.217043._M_start.D.58619._M_p;
> _638 = (long int) _74;
> _261 = -_638;
> _383 = (long unsigned int) _261;
> if (_638 < -8)
>goto ; [90.00%]
> else
>goto ; [10.00%]
>
> _261 : [irange] long int [-9223372036854775807, +INF]
> _383 : [irange] long unsigned int [0,
> 9223372036854775807][9223372036854775809, +INF]
> 8->12 (T) _74 :[irange] _Bit_type * [1, +INF]
> 8->12 (T) _261 : [irange] long int [9, +INF] NONZERO
> 0x7fff
> 8->12 (T) _383 : [irange] long unsigned int [9,
> 9223372036854775807] NONZERO 0x7fff
> 8->12 (T) _638 : [irange] long int [-INF, -9]
>
> === BB 12
> _74 [irange] _Bit_type * [9223372036854775808, 18446744073709551607]
> _383[irange] long unsigned int [9, 9223372036854775807] NONZERO
> 0x7fff
> [local count: 78244465]:
> __builtin_memmove (_216, _74, _383);
>
>
>
> The change is that we now recompute _383 which we didnt before. so we
> are seeing memmove being called on what is effectively:
> memmove (operator new (8), _74, [9, 9223372036854775807])
> And thus the warning.
>
> IS this one of the warnings that has been causing issues? and now Im
> triggering it again?
Yeah, we see these kind of diagnostics on code that's supposed to be
not reachable but we don't figure that out (missed-optimization) or the
code is written in a way that doesn't make this obvious.
>
> Back at fixup_cfg3 time, it looks like:
>
> _261 = __last$D58797$_M_p_245 - _247;
>_262 = _261 > 8;
>_263 = (long int) _262;
>_264 = __builtin_expect (_263, 1);
>if (_264 != 0)
> goto ; [90.00%]
>else
> goto ; [10.00%]
> ..
> [local count: 78244465]:
>_265 = (long unsigned int) _261;
>__builtin_memmove (_246, _247, _265);
>
> So the builtin expect certainly implies it is expecting to have a value > 8
>
> Early on the code looks like:
> _1 = __last_10(D) - __first_11(D);
>_Num_12 = _1 /[ex] 8;
>_2 = _Num_12 > 1;
>_3 = (long int) _2;
>_4 = __builtin_expect (_3, 1);
>if (_4 != 0)
> goto ; [INV]
>else
> goto ; [INV]
>
> :
>_Num.28_5 = (long unsigned int) _Num_12;
>_6 = _Num.28_5 * 8;
>__builtin_memmove (__result_14(D), __first_11(D), _6);
>
>
> SO it does still do basically the same thing.
>
> Im not sure whether this is pointing out something real or another false
> positive...
>
> Andrew
recomputation and PR 109154
The patch, or a slight variation (attached), in the PR allows us to
generate better ranges be recomputing longer instruction sequences on
outgoing edges.
This in fact also fixes
XPASS: gcc.dg/Walloca-13.c (test for bogus messages, line 11)
[local count: 1073741824]:
_1 = p_5(D) - q_6(D);
_2 = _1 /[ex] 4;
n_7 = (long unsigned int) _2;
_11 = (long unsigned int) _1;
if (_11 <= 396)
goto ; [33.00%]
else
goto ; [67.00%]
[local count: 354334800]:
_3 = __builtin_alloca (n_7);
Where _2 was recomputed before, but n_7 was not. Now it is, and we
correctly do not issue the warning any more. awesome.,
however, as seems to be the case often, better ranges result in, I now get:
FAIL: 23_containers/vector/bool/allocator/copy.cc (test for excess errors)
because we now generate:
/opt/notnfs/amacleod/master/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_algobase.h:437:
warning: ‘void* __builtin_memmove(void*, const void*, long unsigned
int)’ writing between 9 and 9223372036854775807 bytes into a region of
size 8 overflows the destination [-Wstringop-overflow=]
I see:
_216 = operator new (8);
_216 : [irange] long unsigned int * [1, +INF]
..
[local count: 86938296]:
D.245552 ={v} {CLOBBER(eol)};
_74 = v1.D.217578._M_impl.D.217043._M_start.D.58619._M_p;
_638 = (long int) _74;
_261 = -_638;
_383 = (long unsigned int) _261;
if (_638 < -8)
goto ; [90.00%]
else
goto ; [10.00%]
_261 : [irange] long int [-9223372036854775807, +INF]
_383 : [irange] long unsigned int [0,
9223372036854775807][9223372036854775809, +INF]
8->12 (T) _74 : [irange] _Bit_type * [1, +INF]
8->12 (T) _261 : [irange] long int [9, +INF] NONZERO
0x7fff
8->12 (T) _383 : [irange] long unsigned int [9,
9223372036854775807] NONZERO 0x7fff
8->12 (T) _638 : [irange] long int [-INF, -9]
=== BB 12
_74 [irange] _Bit_type * [9223372036854775808, 18446744073709551607]
_383 [irange] long unsigned int [9, 9223372036854775807] NONZERO
0x7fff
[local count: 78244465]:
__builtin_memmove (_216, _74, _383);
The change is that we now recompute _383 which we didnt before. so we
are seeing memmove being called on what is effectively:
memmove (operator new (8), _74, [9, 9223372036854775807])
And thus the warning.
IS this one of the warnings that has been causing issues? and now Im
triggering it again?
Back at fixup_cfg3 time, it looks like:
_261 = __last$D58797$_M_p_245 - _247;
_262 = _261 > 8;
_263 = (long int) _262;
_264 = __builtin_expect (_263, 1);
if (_264 != 0)
goto ; [90.00%]
else
goto ; [10.00%]
..
[local count: 78244465]:
_265 = (long unsigned int) _261;
__builtin_memmove (_246, _247, _265);
So the builtin expect certainly implies it is expecting to have a value > 8
Early on the code looks like:
_1 = __last_10(D) - __first_11(D);
_Num_12 = _1 /[ex] 8;
_2 = _Num_12 > 1;
_3 = (long int) _2;
_4 = __builtin_expect (_3, 1);
if (_4 != 0)
goto ; [INV]
else
goto ; [INV]
:
_Num.28_5 = (long unsigned int) _Num_12;
_6 = _Num.28_5 * 8;
__builtin_memmove (__result_14(D), __first_11(D), _6);
SO it does still do basically the same thing.
Im not sure whether this is pointing out something real or another false
positive...
Andrew
commit 358d0ca44faf2e20fbacd0f74386308b5ca52cd4
Author: Andrew MacLeod
Date: Tue Mar 28 12:16:34 2023 -0400
Add recursive GORI recompuations with a depth limit.
diff --git a/gcc/gimple-range-gori.cc b/gcc/gimple-range-gori.cc
index 6e8dfa85ca8..5f4313b27dd 100644
--- a/gcc/gimple-range-gori.cc
+++ b/gcc/gimple-range-gori.cc
@@ -1308,7 +1308,7 @@ gori_compute::compute_operand1_and_operand2_range (vrange ,
// direct dependent is exported, it may also change the computed value of NAME.
bool
-gori_compute::may_recompute_p (tree name, basic_block bb)
+gori_compute::may_recompute_p (tree name, basic_block bb, int depth)
{
tree dep1 = depend1 (name);
tree dep2 = depend2 (name);
@@ -1322,22 +1322,36 @@ gori_compute::may_recompute_p (tree name, basic_block bb)
if (is_a (s) || gimple_has_side_effects (s))
return false;
- // If edge is specified, check if NAME can be recalculated on that edge.
- if (bb)
-return ((is_export_p (dep1, bb))
- || (dep2 && is_export_p (dep2, bb)));
+ if (!dep2)
+{
+ // -1 indicates a default param, convert it to the real default.
+ if (depth == -1)
+ {
+ depth = (int)param_ranger_recompute_depth;
+ gcc_checking_assert (depth >= 1);
+ }
- return (is_export_p (dep1)) || (dep2 && is_export_p (dep2));
+ bool res = (bb ? is_export_p (dep1, bb) : is_export_p (dep1));
+ if (res || depth <= 1)
+ return res;
+ // Check another level of recomputation.
+ return may_recompute_p (dep1, bb, --depth);
+}
+ // Two dependencies terminate the depth of the search.
+
