Re: Regression with recent change

Re: Regression with recent change

[Aldy Hernandez]

Jeff has pointed out that after my change adding global ranges to the
path solver, torture/pr55107.c is failing.  Before I start digging
deep into the IL, I'd like to make sure this is not either expected or
a bogus test.

Compiling this test on x86 with -Wall yields:

$ gcc -c -O2 pr55107.c -Wall
pr55107.c: In function ‘f’:
pr55107.c:32:51: warning: the omitted middle operand in ‘?:’ will
always be ‘true’, suggest explicit middle operand [-Wparentheses]
   32 |       ((a = d) ? b = 0 : (**p ? : 1) != (d != 1 ? : (a = 0)))
!= (k ? a : 0)
      |                                                   ^
pr55107.c:33:11: warning: suggest parentheses around comparison in
operand of ‘!=’ [-Wparentheses]
   32 |       ((a = d) ? b = 0 : (**p ? : 1) != (d != 1 ? : (a = 0)))
!= (k ? a : 0)
      |
  ~~~~~~~~~~~
   33 |           < (a *= c = k) && (**p = 0);
      |           ^~~~~~~~~~~~~~
pr55107.c:33:16: warning: operation on ‘a’ may be undefined [-Wsequence-point]
   33 |           < (a *= c = k) && (**p = 0);
      |                ^~
pr55107.c:33:16: warning: operation on ‘a’ may be undefined [-Wsequence-point]
pr55107.c:33:26: warning: value computed is not used [-Wunused-value]
   33 |           < (a *= c = k) && (**p = 0);
      |                          ^~
pr55107.c:17:14: warning: ‘j’ is used uninitialized [-Wuninitialized]
   17 |       for (; *j; j++)
      |              ^~

So it looks like there's some undefined behavior going on, even before
my patch.  I'd like to get some feedback, because this is usually the
type of problems I see in the presence of a smarter threader... things
get shuffled around, problematic code gets isolated, and warning
passes have an easier time (or sometimes harder time) diagnosing
things.

Thanks.
Aldy

On Fri, Sep 10, 2021 at 10:31 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
> This change:
> 01b5038718056b024b370b74a874fbd92c5bbab3 is the first bad commit
> commit 01b5038718056b024b370b74a874fbd92c5bbab3
> Author: Aldy Hernandez <aldyh@redhat.com>
> Date:   Thu Sep 9 20:30:28 2021 +0200
>
>      Disable threading through latches until after loop optimizations.
>
>      The motivation for this patch was enabling the use of global ranges in
>      the path solver, but this caused certain properties of loops being
>      destroyed which made subsequent loop optimizations to fail.
>      Consequently, this patch's mail goal is to disable jump threading
>      involving the latch until after loop optimizations have run.
> [ ... ]
>
>
> Is causing a regression on nds32le-elf -- perhaps others as well, this
> just happened to pop first in my tester.
>
> Tests that now fail, but worked before (4 tests):
>
> nds32-sim: gcc.dg/torture/pr55107.c   -O2  (test for excess errors)
> nds32-sim: gcc.dg/torture/pr55107.c   -O2  (test for excess errors)
> nds32-sim: gcc.dg/torture/pr55107.c   -O2 -flto -fno-use-linker-plugin
> -flto-partition=none  (test for excess errors)
> nds32-sim: gcc.dg/torture/pr55107.c   -O2 -flto -fno-use-linker-plugin
> -flto-partition=none  (test for excess errors)
>
> I suspect the underlying cause has
> a reasonable chance of causing problems on other ports.
>
>  From the log file:
>
> /home/jlaw/jenkins/workspace/nds32le-elf/gcc/gcc/testsuite/gcc.dg/torture/pr55107.c:
> In function 'f':^M
> /home/jlaw/jenkins/workspace/nds32le-elf/gcc/gcc/testsuite/gcc.dg/torture/pr55107.c:19:21:
> warning: iteration 2147483646 invokes undefined behavior
> [-Waggressive-loop-optimizations]^M
> /home/jlaw/jenkins/workspace/nds32le-elf/gcc/gcc/testsuite/gcc.dg/torture/pr55107.c:19:17:
> note: within this loop^M
>

Re: Regression with recent change

[Jeff Law]

On 9/13/2021 7:29 AM, Aldy Hernandez wrote:
> Jeff has pointed out that after my change adding global ranges to the
> path solver, torture/pr55107.c is failing.  Before I start digging
> deep into the IL, I'd like to make sure this is not either expected or
> a bogus test.
>
> Compiling this test on x86 with -Wall yields:
>
> $ gcc -c -O2 pr55107.c -Wall
> pr55107.c: In function ‘f’:
> pr55107.c:32:51: warning: the omitted middle operand in ‘?:’ will
> always be ‘true’, suggest explicit middle operand [-Wparentheses]
>     32 |       ((a = d) ? b = 0 : (**p ? : 1) != (d != 1 ? : (a = 0)))
> != (k ? a : 0)
>        |                                                   ^
> pr55107.c:33:11: warning: suggest parentheses around comparison in
> operand of ‘!=’ [-Wparentheses]
>     32 |       ((a = d) ? b = 0 : (**p ? : 1) != (d != 1 ? : (a = 0)))
> != (k ? a : 0)
>        |
>    ~~~~~~~~~~~
>     33 |           < (a *= c = k) && (**p = 0);
>        |           ^~~~~~~~~~~~~~
> pr55107.c:33:16: warning: operation on ‘a’ may be undefined [-Wsequence-point]
>     33 |           < (a *= c = k) && (**p = 0);
>        |                ^~
> pr55107.c:33:16: warning: operation on ‘a’ may be undefined [-Wsequence-point]
> pr55107.c:33:26: warning: value computed is not used [-Wunused-value]
>     33 |           < (a *= c = k) && (**p = 0);
>        |                          ^~
> pr55107.c:17:14: warning: ‘j’ is used uninitialized [-Wuninitialized]
>     17 |       for (; *j; j++)
>        |              ^~
>
> So it looks like there's some undefined behavior going on, even before
> my patch.  I'd like to get some feedback, because this is usually the
> type of problems I see in the presence of a smarter threader... things
> get shuffled around, problematic code gets isolated, and warning
> passes have an easier time (or sometimes harder time) diagnosing
> things.
The original issue was PRE hanging, so I'd lean towards keeping the test 
as-is and instead twiddling any warning flags we can to make the 
diagnostics go away.

jeff

Re: Regression with recent change

[Michael Matz]

Hello,

On Mon, 13 Sep 2021, Jeff Law via Gcc-patches wrote:

> > So it looks like there's some undefined behavior going on, even before
> > my patch.  I'd like to get some feedback, because this is usually the
> > type of problems I see in the presence of a smarter threader... things
> > get shuffled around, problematic code gets isolated, and warning
> > passes have an easier time (or sometimes harder time) diagnosing
> > things.
> The original issue was PRE hanging, so I'd lean towards keeping the test as-is
> and instead twiddling any warning flags we can to make the diagnostics go
> away.

Or use this changed test avoiding the issues that I see with -W -Wall on 
this testcase.  I've verified that it still hangs before r194358 and is 
fixed by that revision.

Generally I think, our testsuite, even for ICEs or these kinds of hangs, 
should make an effort to try to write conforming code; if at all possible.  
Here it is possible.

(I don't know if the new threader causes additional warnings, of course, 
but at least the problems with sequence points and uninitialized use of 
'j' aren't necessary to reproduce the bug)


Ciao,
Michael.

/* { dg-do compile } */
/* { dg-additional-options "-fno-split-loops" } */

typedef unsigned short uint16_t;

uint16_t a, b;

int *j_global;
uint16_t f(void)
{
  int c, **p;
  short d = 2, e = 4;

  for (;; b++)
    {
      int *j = j_global, k = 0;

      for (; *j; j++)
	{
	  for(; c; c++)
	    for(; k < 1; k++)
	      {
		short *f = &d;

		if(b)
		  return *f;
	      }
	}

      if(!c)
	d *= e;

      a = d;
      if ((a ? b = 0 : (**p ? : 1) != (d != 1 ? 1 : (b = 0))) != ((k ? a : 0)
	  < (a * (c = k))))
	**p = 0;
    }
}

Re: Regression with recent change

[Jeff Law]

On 9/13/2021 8:18 AM, Michael Matz wrote:
> Hello,
>
> On Mon, 13 Sep 2021, Jeff Law via Gcc-patches wrote:
>
>>> So it looks like there's some undefined behavior going on, even before
>>> my patch.  I'd like to get some feedback, because this is usually the
>>> type of problems I see in the presence of a smarter threader... things
>>> get shuffled around, problematic code gets isolated, and warning
>>> passes have an easier time (or sometimes harder time) diagnosing
>>> things.
>> The original issue was PRE hanging, so I'd lean towards keeping the test as-is
>> and instead twiddling any warning flags we can to make the diagnostics go
>> away.
> Or use this changed test avoiding the issues that I see with -W -Wall on
> this testcase.  I've verified that it still hangs before r194358 and is
> fixed by that revision.
>
> Generally I think, our testsuite, even for ICEs or these kinds of hangs,
> should make an effort to try to write conforming code; if at all possible.
> Here it is possible.
>
> (I don't know if the new threader causes additional warnings, of course,
> but at least the problems with sequence points and uninitialized use of
> 'j' aren't necessary to reproduce the bug)
Well, if we can twiddle the test to remove the undefined behavior 
without compromising its original intent, then that's obviously better -)

jeff

Re: Regression with recent change

[Aldy Hernandez]

On 9/13/21 4:18 PM, Michael Matz wrote:
> Hello,
>
> On Mon, 13 Sep 2021, Jeff Law via Gcc-patches wrote:
>
>>> So it looks like there's some undefined behavior going on, even before
>>> my patch.  I'd like to get some feedback, because this is usually the
>>> type of problems I see in the presence of a smarter threader... things
>>> get shuffled around, problematic code gets isolated, and warning
>>> passes have an easier time (or sometimes harder time) diagnosing
>>> things.
>> The original issue was PRE hanging, so I'd lean towards keeping the test
as-is
>> and instead twiddling any warning flags we can to make the diagnostics go
>> away.
>
> Or use this changed test avoiding the issues that I see with -W -Wall on
> this testcase.  I've verified that it still hangs before r194358 and is
> fixed by that revision.
>
> Generally I think, our testsuite, even for ICEs or these kinds of hangs,
> should make an effort to try to write conforming code; if at all possible.
> Here it is possible.
>
> (I don't know if the new threader causes additional warnings, of course,
> but at least the problems with sequence points and uninitialized use of
> 'j' aren't necessary to reproduce the bug)
>
>
> Ciao,
> Michael.
>
> /* { dg-do compile } */
> /* { dg-additional-options "-fno-split-loops" } */
>
> typedef unsigned short uint16_t;
>
> uint16_t a, b;
>
> int *j_global;
> uint16_t f(void)
> {
>    int c, **p;
>    short d = 2, e = 4;
>
>    for (;; b++)
>      {
>        int *j = j_global, k = 0;
>
>        for (; *j; j++)
>       {
>         for(; c; c++)
>           for(; k < 1; k++)
>             {
>               short *f = &d;
>
>               if(b)
>                 return *f;
>             }
>       }
>
>        if(!c)
>       d *= e;
>
>        a = d;
>        if ((a ? b = 0 : (**p ? : 1) != (d != 1 ? 1 : (b = 0))) != ((k ? a
: 0)
>         < (a * (c = k))))
>       **p = 0;
>      }
> }
>

Thanks for getting rid of the noise here.

I've simplified the above to show what's going on in the warning on
nds32-elf:

int george, *global;
int stuff(), readme();

int
f (void)
{
   int store;

   for (;;)
     {
       int k = 0;

       while (global)
        {
          for (; store; ++store)
            {
              for (; k < 1; k++)
                {
                  if (readme())
                    return 0;
                }
            }
        }

       store = k;
       if (george)
        stuff();
     }
}

The -Waggressive-loop-optimizations pass is complaining because of an
undefined iteration in the for(;store;++store) loop.  But this looks
like it's getting confused by threader having isolated an undefined path.

At the warning, the IL looks like this on entry:

   <bb 2> [local count: 55807730]:
   goto <bb 4>; [100.00%]

   <bb 4> [local count: 57254340]:
   # store_4 = PHI <k_42(3), store_14(D)(2)>
   global.0_25 = global;
   if (global.0_25 != 0B)
     goto <bb 12>; [94.50%]
   else
     goto <bb 13>; [5.50%]

...
...

  <bb 12> [local count: 54105352]:
   if (store_4 != 0)
     goto <bb 7>; [99.64%]
   else
     goto <bb 10>; [0.36%]

If global.0_25 was true on entry, the read from store_4 would be
undefined.  Presumably the warning pass is assuming this path always
gets executed.

This looks like a latent bug.  For that matter, the above snippet warns
with -fdisable-tree-thread2, even on x86-64 (and before my
patch).

Aldy

Re: Regression with recent change

[Michael Matz]

Hello,

On Mon, 13 Sep 2021, Aldy Hernandez via Gcc-patches wrote:

The testcase still tests what it's supposed to test with ...

> > typedef unsigned short uint16_t;
> >
> > uint16_t a, b;
> >
> > int *j_global;
> > uint16_t f(void)
> > {
> >    int c, **p;
> >    short d = 2, e = 4;
> >

... "c = a;" added here (i.e. it still hangs before the pr55107 change).

> >    for (;; b++)
> >      {
> >        int *j = j_global, k = 0;
> >
> >        for (; *j; j++)
> >       {
> >         for(; c; c++)
> >           for(; k < 1; k++)
> >             {
> >               short *f = &d;
> >
> >               if(b)
> >                 return *f;
> >             }
> >       }
> >
> >        if(!c)
> >       d *= e;
> >
> >        a = d;
> >        if ((a ? b = 0 : (**p ? : 1) != (d != 1 ? 1 : (b = 0))) != ((k ? a
> : 0)
> >         < (a * (c = k))))
> >       **p = 0;
> >      }
> > }
> >
> 
> Thanks for getting rid of the noise here.
> 
> I've simplified the above to show what's going on in the warning on
> nds32-elf:
> 
> int george, *global;
> int stuff(), readme();
> 
> int
> f (void)
> {
>    int store;
> 
>    for (;;)
>      {
>        int k = 0;
> 
>        while (global)
>         {
>           for (; store; ++store)

Yeah, that seems a correct warning, your 'store' (the 'c' in the original 
testcase) is really used uninitialized (when 'global' aka '*j_global' is 
non-zero).  Sorry for not noticing earlier, I was only getting rid of 
warnings, not carefully looking at the testcase itself.  I think with 
above initialization of 'c' it's conforming, and still a correct test for 
the original bug.

> This looks like a latent bug.  For that matter, the above snippet warns 
> with -fdisable-tree-thread2, even on x86-64 (and before my patch).


Ciao,
Michael.

Re: Regression with recent change

[Aldy Hernandez]

On 9/14/21 4:13 PM, Michael Matz wrote:
> Hello,
> 
> On Mon, 13 Sep 2021, Aldy Hernandez via Gcc-patches wrote:
> 
> The testcase still tests what it's supposed to test with ...
> 
>>> typedef unsigned short uint16_t;
>>>
>>> uint16_t a, b;
>>>
>>> int *j_global;
>>> uint16_t f(void)
>>> {
>>>     int c, **p;
>>>     short d = 2, e = 4;
>>>
> 
> ... "c = a;" added here (i.e. it still hangs before the pr55107 change).
> 
>>>     for (;; b++)
>>>       {
>>>         int *j = j_global, k = 0;
>>>
>>>         for (; *j; j++)
>>>        {
>>>          for(; c; c++)
>>>            for(; k < 1; k++)
>>>              {
>>>                short *f = &d;
>>>
>>>                if(b)
>>>                  return *f;
>>>              }
>>>        }
>>>
>>>         if(!c)
>>>        d *= e;
>>>
>>>         a = d;
>>>         if ((a ? b = 0 : (**p ? : 1) != (d != 1 ? 1 : (b = 0))) != ((k ? a
>> : 0)
>>>          < (a * (c = k))))
>>>        **p = 0;
>>>       }
>>> }
>>>
>>
>> Thanks for getting rid of the noise here.
>>
>> I've simplified the above to show what's going on in the warning on
>> nds32-elf:
>>
>> int george, *global;
>> int stuff(), readme();
>>
>> int
>> f (void)
>> {
>>     int store;
>>
>>     for (;;)
>>       {
>>         int k = 0;
>>
>>         while (global)
>>          {
>>            for (; store; ++store)
> 
> Yeah, that seems a correct warning, your 'store' (the 'c' in the original
> testcase) is really used uninitialized (when 'global' aka '*j_global' is
> non-zero).  Sorry for not noticing earlier, I was only getting rid of
> warnings, not carefully looking at the testcase itself.  I think with
> above initialization of 'c' it's conforming, and still a correct test for
> the original bug.

This is good news.  So far, every single warning/problem that has 
surfaced with the backward threader rewrite has been a latent bug 
elsewhere (or a bad test) :-).

Would you mind checking in your changes to the testcase?

Thanks.
Aldy

Re: Regression with recent change

[Jeff Law]

On 9/14/2021 8:53 AM, Aldy Hernandez wrote:
>
>
> On 9/14/21 4:13 PM, Michael Matz wrote:
>> Hello,
>>
>> On Mon, 13 Sep 2021, Aldy Hernandez via Gcc-patches wrote:
>>
>> The testcase still tests what it's supposed to test with ...
>>
>>>> typedef unsigned short uint16_t;
>>>>
>>>> uint16_t a, b;
>>>>
>>>> int *j_global;
>>>> uint16_t f(void)
>>>> {
>>>>     int c, **p;
>>>>     short d = 2, e = 4;
>>>>
>>
>> ... "c = a;" added here (i.e. it still hangs before the pr55107 change).
>>
>>>>     for (;; b++)
>>>>       {
>>>>         int *j = j_global, k = 0;
>>>>
>>>>         for (; *j; j++)
>>>>        {
>>>>          for(; c; c++)
>>>>            for(; k < 1; k++)
>>>>              {
>>>>                short *f = &d;
>>>>
>>>>                if(b)
>>>>                  return *f;
>>>>              }
>>>>        }
>>>>
>>>>         if(!c)
>>>>        d *= e;
>>>>
>>>>         a = d;
>>>>         if ((a ? b = 0 : (**p ? : 1) != (d != 1 ? 1 : (b = 0))) != 
>>>> ((k ? a
>>> : 0)
>>>>          < (a * (c = k))))
>>>>        **p = 0;
>>>>       }
>>>> }
>>>>
>>>
>>> Thanks for getting rid of the noise here.
>>>
>>> I've simplified the above to show what's going on in the warning on
>>> nds32-elf:
>>>
>>> int george, *global;
>>> int stuff(), readme();
>>>
>>> int
>>> f (void)
>>> {
>>>     int store;
>>>
>>>     for (;;)
>>>       {
>>>         int k = 0;
>>>
>>>         while (global)
>>>          {
>>>            for (; store; ++store)
>>
>> Yeah, that seems a correct warning, your 'store' (the 'c' in the 
>> original
>> testcase) is really used uninitialized (when 'global' aka '*j_global' is
>> non-zero).  Sorry for not noticing earlier, I was only getting rid of
>> warnings, not carefully looking at the testcase itself.  I think with
>> above initialization of 'c' it's conforming, and still a correct test 
>> for
>> the original bug.
>
> This is good news.  So far, every single warning/problem that has 
> surfaced with the backward threader rewrite has been a latent bug 
> elsewhere (or a bad test) :-).
Always good news :-)

>
> Would you mind checking in your changes to the testcase?
Yes, please do.

jeff

Regression with recent change

[Jeff Law]

This change:

985b3a6837dee7001e6b618f073ed74f0edf5787 is the first bad commit
commit 985b3a6837dee7001e6b618f073ed74f0edf5787
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Mon Jun 10 09:57:15 2019 -0700

     Generate offset adjusted operation for op_by_pieces operations

     Add an overlap_op_by_pieces_p target hook for op_by_pieces operations
     between two areas of memory to generate one offset adjusted operation
     in the smallest integer mode for the remaining bytes on the last piece
     operation of a memory region to avoid doing more than one smaller
     operations.

     Pass the RTL information from the previous iteration to m_constfn in
     op_by_pieces operation so that builtin_memset_[read|gen]_str can
     generate the new RTL from the previous RTL.

     Tested on Linux/x86-64.

     gcc/

             PR middle-end/90773
             * builtins.c (builtin_memcpy_read_str): Add a dummy argument.
             (builtin_strncpy_read_str): Likewise.
             (builtin_memset_read_str): Add an argument for the previous RTL
             information and generate the new RTL from the previous RTL 
info.
             (builtin_memset_gen_str): Likewise.
             * builtins.h (builtin_strncpy_read_str): Update the prototype.
             (builtin_memset_read_str): Likewise.
             * expr.c (by_pieces_ninsns): If 
targetm.overlap_op_by_pieces_p()
             returns true, round up size and alignment to the widest integer
             mode for maximum size.
             (pieces_addr::adjust): Add a pointer to by_pieces_prev argument
             and pass it to m_constfn.
             (op_by_pieces_d): Add m_push and m_overlap_op_by_pieces.
             (op_by_pieces_d::op_by_pieces_d): Add a bool argument to
             initialize m_push.  Initialize m_overlap_op_by_pieces with
             targetm.overlap_op_by_pieces_p ().
             (op_by_pieces_d::run): Pass the previous RTL information to
             pieces_addr::adjust and generate overlapping operations if
             m_overlap_op_by_pieces is true.
             (PUSHG_P): New.
             (move_by_pieces_d::move_by_pieces_d): Updated for 
op_by_pieces_d
             change.
             (store_by_pieces_d::store_by_pieces_d): Updated for 
op_by_pieces_d
             change.
             (can_store_by_pieces): Use by_pieces_constfn on constfun.
             (store_by_pieces): Use by_pieces_constfn on constfun. Updated
             for op_by_pieces_d change.
             (clear_by_pieces_1): Add a dummy argument.
             (clear_by_pieces): Updated for op_by_pieces_d change.
             (compare_by_pieces_d::compare_by_pieces_d): Likewise.
             (string_cst_read_str): Add a dummy argument.
             * expr.h (by_pieces_constfn): Add a dummy argument.
             (by_pieces_prev): New.
             * target.def (overlap_op_by_pieces_p): New target hook.
             * config/i386/i386.c (TARGET_OVERLAP_OP_BY_PIECES_P): New.
             * doc/tm.texi.in: Add TARGET_OVERLAP_OP_BY_PIECES_P.
             * doc/tm.texi: Regenerated.


Is causing regressions on the fr30-elf port:


fr30-sim: gcc.dg/Wstringop-overflow-27.c  (test for warnings, line 168)
fr30-sim: gcc.dg/Wstringop-overflow-27.c  (test for warnings, line 174)
fr30-sim: gcc.dg/Wstringop-overflow-27.c  (test for warnings, line 180)
fr30-sim: gcc.dg/Wstringop-overflow-27.c  (test for warnings, line 186)
fr30-sim: gcc.dg/Wstringop-overflow-27.c  (test for warnings, line 193)
fr30-sim: gcc.dg/Wstringop-overflow-27.c  (test for warnings, line 201)
fr30-sim: gcc.dg/Wstringop-overflow-27.c  (test for warnings, line 265)
fr30-sim: gcc.dg/Wstringop-overflow-27.c  (test for warnings, line 274)
fr30-sim: gcc.dg/Wstringop-overflow-27.c  (test for warnings, line 282)
fr30-sim: gcc.dg/Wstringop-overflow-27.c  (test for warnings, line 289)
fr30-sim: gcc.dg/Wstringop-overflow-27.c (test for excess errors)
fr30-sim: gcc.dg/Wstringop-overflow-27.c malloc note (test for warnings, 
line 288)
fr30-sim: gcc.dg/Wstringop-overflow-27.c note (test for warnings, line 264)
fr30-sim: gcc.dg/Wstringop-overflow-27.c note (test for warnings, line 273)
fr30-sim: gcc.dg/Wstringop-overflow-27.c vla note (test for warnings, 
line 281)


I haven't done any analysis other then bisecting the regression.


Full logs are here:

http://3.14.90.209:8080/job/fr30-elf/1258/console


Thanks,

Jeff