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)

   <bb 2> [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 <bb 3>; [33.00%]
   else
     goto <bb 4>; [67.00%]

   <bb 3> [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:
<BB 2>
  ....
     _216 = operator new (8);

_216 : [irange] long unsigned int * [1, +INF]
   ......

     <bb 8> [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 <bb 12>; [90.00%]
     else
       goto <bb 13>; [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 
0x7fffffffffffffff
8->12  (T) _383 :       [irange] long unsigned int [9, 
9223372036854775807] NONZERO 0x7fffffffffffffff
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 
0x7fffffffffffffff
     <bb 12> [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 <bb 47>; [90.00%]
   else
     goto <bb 48>; [10.00%]
..................
   <bb 47> [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 <bb 3>; [INV]
   else
     goto <bb 4>; [INV]

   <bb 3> :
   _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
  

On Wed, Mar 29, 2023 at 7:22 PM Andrew MacLeod <amacleod@redhat.com> 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)
>
>    <bb 2> [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 <bb 3>; [33.00%]
>    else
>      goto <bb 4>; [67.00%]
>
>    <bb 3> [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:
> <BB 2>
>   ....
>      _216 = operator new (8);
>
> _216 : [irange] long unsigned int * [1, +INF]
>    ......
>
>      <bb 8> [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 <bb 12>; [90.00%]
>      else
>        goto <bb 13>; [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
> 0x7fffffffffffffff
> 8->12  (T) _383 :       [irange] long unsigned int [9,
> 9223372036854775807] NONZERO 0x7fffffffffffffff
> 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
> 0x7fffffffffffffff
>      <bb 12> [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 <bb 47>; [90.00%]
>    else
>      goto <bb 48>; [10.00%]
> ..................
>    <bb 47> [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 <bb 3>; [INV]
>    else
>      goto <bb 4>; [INV]
>
>    <bb 3> :
>    _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
  

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 <amacleod@redhat.com>
> 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 &r,
>  // 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<gphi *> (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 &op1_range,
>  			      tree op2, vrange &op2_range,
>  			      fur_source &src, 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 &r, gswitch *s, const vrange &lhs,
>  				     tree name, fur_source &src);
>    bool compute_operand1_range (vrange &r, gimple_range_op_handler &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, 9999) Param Optimization
>  Maximum number of relations the oracle will register in a basic block.


	Jakub
  

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 <amacleod@redhat.com>
>> 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
  

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 <amacleod@redhat.com>
> > > 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 <amacleod@redhat.com>
> 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
  

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, &MEM <const char[37]> [(void 
*)"0123456789abcdefghijklmnopqrstuvwxyz" + 34B], 3);

Now, by VRP2 we have figured it out...

     _40 = operator new [] (0);
     __builtin_memcpy (_40, &MEM <const char[37]> [(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 <amacleod@redhat.com>
>>>> 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 <amacleod@redhat.com>
>> 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
>
  

On Thu, Mar 30, 2023 at 1:40 PM Andrew MacLeod via Gcc-patches
<gcc-patches@gcc.gnu.org> 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 <command-line>:^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<bool, _Alloc>::iterator
std::vector<bool, _Alloc>::_M_copy_aligned(const_iterator,
const_iterator, iterator) [with _Alloc =
__gnu_test::propagating_allocator<bool, false>]' 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<bool,
_Alloc>::_M_insert_aux(iterator, bool) [with _Alloc =
__gnu_test::propagating_allocator<bool, false>]' 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<bool, _Alloc>::push_back(bool)
[with _Alloc = __gnu_test::propagating_allocator<bool, false>]' 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<std::allocator<_CharT>
>::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<Tp, Alloc>::pointer
__gnu_test::uneq_allocator<Tp, Alloc>::allocate(size_type, const
void*) [with Tp = long unsigned int; Alloc = std::allocator<long
unsigned int>]' at
/home/apinski/src/upstream-gcc/gcc/libstdc++-v3/testsuite/util/testsuite_allocator.h:360,^M
    inlined from 'static std::allocator_traits<
<template-parameter-1-1> >::pointer std::allocator_traits<
<template-parameter-1-1> >::allocate(_Alloc&, size_type) [with _Alloc
= __gnu_test::propagating_allocator<long unsigned int, false,
std::allocator<long unsigned int> >]' 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<bool, false>]' 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<bool,
_Alloc>::_M_insert_aux(iterator, bool) [with _Alloc =
__gnu_test::propagating_allocator<bool, false>]' at
/home/apinski/src/upstream-gcc/gcc/objdir/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/vector.tcc:943,^M
    inlined from 'void std::vector<bool, _Alloc>::push_back(bool)
[with _Alloc = __gnu_test::propagating_allocator<bool, false>]' 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/new_allocator.h:147:
note: destination object of size 8 allocated by 'operator new'^M


Thanks,
Andrew Pinski


Thanks,
Andrew Pinski

>
> 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, &MEM <const char[37]> [(void
> *)"0123456789abcdefghijklmnopqrstuvwxyz" + 34B], 3);
>
> Now, by VRP2 we have figured it out...
>
>      _40 = operator new [] (0);
>      __builtin_memcpy (_40, &MEM <const char[37]> [(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 <amacleod@redhat.com>
> >>>> 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 <amacleod@redhat.com>
> >> 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
> >
>
  

> 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
  

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
  

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 <bb 9>; [INV]

   <bb 3> :
   if (e.0_1 > 
5.00000000000000010408340855860842566471546888351440429688e-3)
     goto <bb 4>; [INV]
   else
     goto <bb 8>; [INV]

   <bb 4> :
   if (E_13 > 1.0e+0)
     goto <bb 5>; [INV]
   else
     goto <bb 6>; [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:

  <bb 2> [local count: 357878152]:
   e.0_1 = e;
   if (e.0_1 > 
5.00000000000000010408340855860842566471546888351440429688e-3)
     goto <bb 3>; [50.00%]
   else
     goto <bb 4>; [50.00%]

   <bb 3> [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.0000000000000001942890293094023945741355419158935546875e-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
  

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
  

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
>
  

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
  

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
  

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
  

> Date: Fri, 31 Mar 2023 15:48:22 -0400
> From: Andrew MacLeod via Gcc-patches <gcc-patches@gcc.gnu.org>
> Reply-To: Andrew MacLeod <amacleod@redhat.com>

> commit 55bf4f0d443e5adbacfcdbbebf4b2e0c74d1dcc8
> Author: Andrew MacLeod <amacleod@redhat.com>
> 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
  

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 <gcc-patches@gcc.gnu.org>
>> Reply-To: Andrew MacLeod <amacleod@redhat.com>
>> commit 55bf4f0d443e5adbacfcdbbebf4b2e0c74d1dcc8
>> Author: Andrew MacLeod <amacleod@redhat.com>
>> 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