[Bug c/102844] New: gcc-9.x miscompiles unsigned char expression in switch

[Bug c/102844] New: gcc-9.x miscompiles unsigned char expression in switch

[breiten at lexmark dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

            Bug ID: 102844
           Summary: gcc-9.x miscompiles unsigned char expression in switch
           Product: gcc
           Version: 9.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: breiten at lexmark dot com
  Target Milestone: ---

The code included below demonstrates a code-generation bug we found in gcc-9.3
(built with poky's dunfell branch) It is present in all of the 9.x releases,
but none of the 10.x releases.

Anticipating your first question, I found that this problem is NOT present
in gcc-10.2, so I went earlier in that branch and was able to see that it
was corrected by 744fd446c321f78f9a1ce4ef5f83df8dcfa44a9 (gcc-10 branch,
but before gcc-10.1 release) Nothing in the commit messages suggests a
bug fix.

I've also been able to pinpoint the commit where the problem first appears
in the gcc-9 branch: 1a438d160e1dc845882c2ecba99513a09a931623.  Here, the
commit message describes this commit to be a bug fix.

My dilemma is that I'm stuck in the 9 branch for a while.  Reverting a bug
fix doesn't feel right. But while picking up the commit from gcc-10 does
appear to work, it a bit more than a simple bug-fix, and I'd like to hear
some opinions regarding its appropriateness.

Maybe there it still a bug out there, and the commit from gcc-10 just makes
this particular code compile properly.

In the C source below, line 58 miscompiles into an unconditional store of
0x80.  Notice that I have two switch statements on the same variable and
with the same cases.  If I remove or alter the other one, the bad code and
the warning goes away.  It also is only present when "nd - 0x80" is stored
to a uchar variable.  If its stored to an int or printf'd the code is correct.
Also, adding "U" suffix to 0x80 corrects the expression.  And I get the warning
regardless of what number I subtract from nd, suggesting that it is incorrectly
considered a signed char, and subtracting anything would underflow.

I see the problem with -Os or -O2, and -fno-strict-overflow is needed.  In
cleaning up the command line, I found that linux adds -Wno-maybe-uninitialized.
Removing that leads to this message:

attach3.c:60:22: warning: '({anonymous})' may be used uninitialized in this
function [-Wmaybe-uninitialized]
   58 |             *ic = nd - 0x80; /* compiles into an unconditional store of
0x80 */
      |                   ~~~^~~~~~

Here's my gcc configuration:
Target: arm-poky-linux-gnueabi
Configured with: ../../../../../../work-shared/gcc-9.3.0-r0/gcc-9.3.0/configure
--build=x86_64-linux --host=x86_64-linux --target=arm-poky-linux-gnueabi
--prefix=/host-native/usr --exec_prefix=/host-native/usr
--bindir=/host-native/usr/bin/arm-poky-linux-gnueabi
--sbindir=/host-native/usr/bin/arm-poky-linux-gnueabi
--libexecdir=/host-native/usr/libexec/arm-poky-linux-gnueabi
--datadir=/host-native/usr/share --sysconfdir=/host-native/etc
--sharedstatedir=/host-native/com --localstatedir=/host-native/var
--libdir=/host-native/usr/lib/arm-poky-linux-gnueabi
--includedir=/host-native/usr/include --oldincludedir=/host-native/usr/include
--infodir=/host-native/usr/share/info --mandir=/host-native/usr/share/man
--disable-silent-rules --disable-dependency-tracking
--with-libtool-sysroot=/host-native --enable-clocale=generic --with-gnu-ld
--enable-shared --enable-languages=c,c++ --enable-threads=posix
--disable-multilib --enable-default-pie --enable-c99 --enable-long-long
--enable-symvers=gnu --enable-libstdcxx-pch
--program-prefix=arm-poky-linux-gnueabi- --without-local-prefix
--disable-install-libiberty --enable-lto --disable-libssp --enable-libitm
--disable-bootstrap --disable-libmudflap --with-system-zlib
--with-linker-hash-style=sysv --enable-linker-build-id --with-ppl=no
--with-cloog=no --enable-checking=release --enable-cheaders=c_global
--without-isl --with-gxx-include-dir=/not/exist/usr/include/c++/9.3.0
--with-sysroot=/not/exist --with-build-sysroot=/host
--enable-poison-system-directories --with-system-zlib --disable-static
--disable-nls --with-glibc-version=2.28 --enable-initfini-array
Thread model: posix
gcc version 9.3.0 (GCC)


<< code to demonstrate problem >>
typedef struct
{
  void (*request)(int request, void *arg);
} po_t;

po_t *po;
unsigned char *ambp(void);
unsigned char da;
unsigned char dic;

unsigned char gnda(unsigned char current_address, int sequence);
void rnsm(void);
int state = 0;
static unsigned char *attach_mb_ptr;
static unsigned char ta;
static unsigned char lda;

static unsigned char snod(unsigned char cd, unsigned char *tl, unsigned char
*ic)
{
   unsigned char nd;
   while ((nd = gnda(cd,0)) != 0x21)
   {
      int Req;
      po->request(57, &Req);
      if (!Req)
         break;
   }
   if (nd != 0x21)
   {
      switch (nd)
      {
         case 0x95:
            *tl = 0x01;
            break;

         case 0x80:
         case 0x81:
         case 0x82:
            *tl = 0x00;
            break;

         default:
            *tl = 0x05;
            break;
      }
   }
   if (nd != 0x21)
   {
      switch (nd)
      {
         case 0x95:
             *ic = nd - 0x91;
            break;

         case 0x80:
         case 0x81:
         case 0x82:
            *ic = nd - 0x80;    /* compiles into an unconditional store of 0x80
*/
            break;

         default:
            *ic = 0;
            break;
      }
   }
   return nd;
}

void rosm3(unsigned char reason)
{
   switch(state)
   {
      case 0:
           attach_mb_ptr = ambp();
           da = snod(0x20, &ta, &dic);
           break;
      case 1:
           lda = da;
           if ((da = snod(lda, &ta, &dic)) != 0x21)
              if (reason == 1)
                 rnsm();
           break;
   }
}

[Bug tree-optimization/102844] [9/10/11/12 Regression] gcc-9.x miscompiles unsigned char expression in switch

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Target Milestone|---                         |9.5
   Last reconfirmed|                            |2021-10-19
            Summary|gcc-9.x miscompiles         |[9/10/11/12 Regression]
                   |unsigned char expression in |gcc-9.x miscompiles
                   |switch                      |unsigned char expression in
                   |                            |switch
     Ever confirmed|0                           |1

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
So the problem first shows up in .vrp2.
  nd.7_11 = _34(D);
  _12 = _33(D) + -128;
  _13 = (unsigned char) _12;

Before:
  <bb 13> [local count: 25265782]:
<L4>:
  nd.7_11 = _46;
  _12 = _46 + -128;
  _13 = (unsigned char) _12;
  dic = _13;

This happens during the insertation of the assert statements.
I don't think there has been any known issues like this before.

[Bug tree-optimization/102844] [9/10/11/12 Regression] gcc-9.x miscompiles unsigned char expression in switch

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Before:
  <bb 13> [local count: 25265782]:
<L4>:
  nd.7_11 = _46;
  _12 = _46 + -128;
  _13 = (unsigned char) _12;
  dic = _13;
  goto <bb 15>; [100.00%]

  <bb 14> [local count: 25265782]:
<L5>:
  dic = 0;

  <bb 15> [local count: 114848370]:
  # nd_52 = PHI <nd_6(14), 149(12), nd_6(13), 33(8)>
  return nd_52;

  <bb 16> [local count: 25270028]:
  _46 = (signed char) nd_6;
  if (_46 < 0)
    goto <bb 13>; [33.33%]
  else
    goto <bb 14>; [66.67%]


Hmmm:

  <bb 16> [local count: 25270028]:
  nd_36 = ASSERT_EXPR <nd_47, nd_47 <= 130>;
  _46 = (signed char) nd_36;
  if (_46 < 0)
    goto <bb 21>; [33.33%]
  else
    goto <bb 20>; [66.67%]


  <bb 21> [local count: 8423438]:
  _17 = ASSERT_EXPR <_46, _46 < 0>;
  nd_39 = ASSERT_EXPR <nd_36, nd_36 + 128 <= 127>;

  <bb 13> [local count: 25265782]:
<L4>:
  nd.7_11 = _34(D);
  _12 = _33(D) + -128;
  _13 = (unsigned char) _12;
  dic = _13;
  goto <bb 15>; [100.00%]

  <bb 20> [local count: 16846590]:
  nd_45 = ASSERT_EXPR <nd_36, nd_36 + 128 > 127>;


Hmm, no phi node at bb 13 for _46 ....

[Bug tree-optimization/102844] [9/10/11/12 Regression] gcc-9.x miscompiles unsigned char expression in switch

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
> 1a438d160e1dc845882c2ecba99513a09a931623

It was definitely latent before this patch.

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[9/10/11/12 Regression]     |[9/10/11/12 Regression] vrp
                   |gcc-9.x miscompiles         |inserting assert causes
                   |unsigned char expression in |miscompiling when doing
                   |switch                      |update_ssa

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
      update_ssa (TODO_update_ssa_no_phi);
Hmm, but there needs to be a phi node here ....

Someone who understands VRP's insertation of the asserts and update_ssa better
than me should look into this issue.

Note this is definitely latent on the trunk but can be reproduced on the GCC 9
branch.

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
VRP does intentionally not insert PHIs, let me have a look (and yes, both
bisections just make the bug change from latent to not latent or vice versa).

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |needs-bisection

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
So the issue is that the SSA form is invalid at the start of VRP2 - we have

<bb 16> [local count: 25270028]:
_45 = (signed char) nd_5;
if (_45 < 0)
  goto <bb 13>; [33.33%]
else
  goto <bb 14>; [66.67%]

<bb 10> [local count: 25267450]:
<L1>:
ta = 0;
goto <bb 13>; [100.00%]

<bb 13> [local count: 25265782]:
<L4>:
nd.7_10 = _45;
_11 = _45 + -128;
_12 = (unsigned char) _11;
dic = _12;
goto <bb 15>; [100.00%]

but there's a PHI missing for _45 - the definition site does not dominate the
use.

(gdb) p verify_ssa (true, true)
t.c: In function 'snod.constprop':
t.c:18:22: error: definition in block 16 does not dominate use in block 13
   18 | static unsigned char snod(unsigned char cd, unsigned char *tl, unsigned
char *ic)
      |                      ^~~~
for SSA_NAME: _45 in statement:
nd.7_10 = _45;

and the issue pops up after DOM3.

It would be nice to bisect with checking enabled or -fchecking - Martin?

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Btw, the ICE also reproduces on native x86_64, not just with a cross to arm and
both GCC 8 and GCC 10 look fine in this regard.

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|needs-bisection             |

--- Comment #8 from Martin Liška <marxin at gcc dot gnu.org> ---
Using x86_64 compiler, the issue is fixed on master after
r10-1420-g744fd446c321f78f and started with r9-6384-g1a438d160e1dc845.

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #8)
> Using x86_64 compiler, the issue is fixed on master after
> r10-1420-g744fd446c321f78f and started with r9-6384-g1a438d160e1dc845.

Can you continue searching for a fix with -fdisable-tree-fre4 (r10-1420
added FRE4 before DOM3).

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #10 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #9)
> (In reply to Martin Liška from comment #8)
> > Using x86_64 compiler, the issue is fixed on master after
> > r10-1420-g744fd446c321f78f and started with r9-6384-g1a438d160e1dc845.
> 
> Can you continue searching for a fix with -fdisable-tree-fre4 (r10-1420
> added FRE4 before DOM3).

Sure, then it gets fixed with r11-408-g84935c9822183ce4.

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |law at gcc dot gnu.org

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
OK, so the issue is that we are doing a jump threading that violates SSA form
since it bypasses the definition used by the merge block.  There's likely
validation that this cannot happen but since we're forward-threading it's
probably happening too early and the SSA definition is only apperaring when
visiting a block we thread through where we change

   if (nd_5 >= 128)

to

   _46 = (singed char) nd_6;
   if (_46 < 0)

and the merge had

   L4:
    nd.7_11 = (signed char) nd_5;

where we now realize the CSE opportunity to _46.

The r9-6384-g1a438d160e1dc845 change makes us enter the new _46 = (signed char)
nd_6 expression into the hashtable (it was skipped before).

Somebody more familiar with the (old) threading code needs to debug this
further or provide hints as to where the designed point is to avoid this issue.
 Jeff?

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #10)
> (In reply to Richard Biener from comment #9)
> > (In reply to Martin Liška from comment #8)
> > > Using x86_64 compiler, the issue is fixed on master after
> > > r10-1420-g744fd446c321f78f and started with r9-6384-g1a438d160e1dc845.
> > 
> > Can you continue searching for a fix with -fdisable-tree-fre4 (r10-1420
> > added FRE4 before DOM3).
> 
> Sure, then it gets fixed with r11-408-g84935c9822183ce4.

-fno-tree-sink?

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aldyh at gcc dot gnu.org

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
Aldyh might also have stumbled over code avoiding to thread across a SSA
definition.

Ah - I guess the block is recorded as "forwarder" (block with just a condition
we are able to thread through) but it becomes a non-"forwarder" because later
a SSA definition is inserted but the actual threading code will not copy
those stmts (or verify they impose no problem)?

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #14 from Richard Biener <rguenth at gcc dot gnu.org> ---
  Registering jump thread: (10, 16) incoming edge;  (16, 17) normal; (17, 13)
nocopy;

  <bb 6> [local count: 75809931]:
  if (nd_5 >= 128)
    goto <bb 10>; [40.00%]
  else
    goto <bb 11>; [60.00%]

  <bb 10> [local count: 25267450]:
<L1>:
  ta = 0;
  goto <bb 16>; [100.00%]

  <bb 16> [local count: 50537478]:
  if (nd_5 > 130)
    goto <bb 18>; [25.00%]
  else
    goto <bb 17>; [75.00%]

  <bb 17> [local count: 50537478]:
  if (nd_5 >= 128)
    goto <bb 13>; [66.67%]
  else
    goto <bb 14>; [33.33%]

  <bb 13> [local count: 25265782]:
<L4>:
  nd.7_11 = (signed char) nd_5;
  _12 = nd.7_11 + -128;
  _13 = (unsigned char) _12;
  dic = _13;
  goto <bb 15>; [100.00%]

and bb 17 is where we later insert the stmt and it is indeed marked as
'nocopy'.

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> ---
The following removes the premature optimization(?) using
EDGE_NO_COPY_SRC_BLOCK
in case the block contained a condition we could thread through.  That fixes
the testcase.

diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
index c3ea2d680d8..2005e30322b 100644
--- a/gcc/tree-ssa-threadedge.c
+++ b/gcc/tree-ssa-threadedge.c
@@ -989,8 +989,11 @@ thread_around_empty_blocks (edge taken_edge,
        return false;
       bitmap_set_bit (visited, taken_edge->dest->index);

+      /* We may not use EDGE_NO_COPY_SRC_BLOCK since the condition
+        might later be simplified to something needing a temporary
+        SSA definition.  See PR102844.  */
       jump_thread_edge *x
-       = new jump_thread_edge (taken_edge, EDGE_NO_COPY_SRC_BLOCK);
+       = new jump_thread_edge (taken_edge, EDGE_COPY_SRC_BLOCK);
       path->safe_push (x);

       thread_around_empty_blocks (taken_edge,

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #16 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #13)
> Aldyh might also have stumbled over code avoiding to thread across a SSA
> definition.
> 
> Ah - I guess the block is recorded as "forwarder" (block with just a
> condition
> we are able to thread through) but it becomes a non-"forwarder" because later
> a SSA definition is inserted but the actual threading code will not copy
> those stmts (or verify they impose no problem)?

I haven't run into this in the forward threader, but I have run into something
similar in the new path solver (which I fixed).  But that's unlikely to help
you here.

Silly question, why is the SSA form invalid on entry to VRP2?  That's just
asking for trouble.  Is this related to how asserts work?

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #17 from rguenther at suse dot de <rguenther at suse dot de> ---
On Wed, 20 Oct 2021, aldyh at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844
> 
> --- Comment #16 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
> (In reply to Richard Biener from comment #13)
> > Aldyh might also have stumbled over code avoiding to thread across a SSA
> > definition.
> > 
> > Ah - I guess the block is recorded as "forwarder" (block with just a
> > condition
> > we are able to thread through) but it becomes a non-"forwarder" because later
> > a SSA definition is inserted but the actual threading code will not copy
> > those stmts (or verify they impose no problem)?
> 
> I haven't run into this in the forward threader, but I have run into something
> similar in the new path solver (which I fixed).  But that's unlikely to help
> you here.
> 
> Silly question, why is the SSA form invalid on entry to VRP2?  That's just
> asking for trouble.  Is this related to how asserts work?

Well, DOM threading creates invalid SSA (definition not dominating use).
Doesn't have to do anything with VRP or asserts.

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amacleod at redhat dot com

--- Comment #18 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to rguenther@suse.de from comment #17)
> On Wed, 20 Oct 2021, aldyh at gcc dot gnu.org wrote:

> > Silly question, why is the SSA form invalid on entry to VRP2?  That's just
> > asking for trouble.  Is this related to how asserts work?
> 
> Well, DOM threading creates invalid SSA (definition not dominating use).
> Doesn't have to do anything with VRP or asserts.

Ah, I see.

BTW, if this is still the case in mainline, this is bound to be a problem for
the ranger.  Andrew, won't we get an UNDEFINED / unreachable if we query the
non dominating use at this point?

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org
             Status|ASSIGNED                    |NEW
           Priority|P3                          |P2
           Assignee|rguenth at gcc dot gnu.org         |unassigned at gcc dot gnu.org

--- Comment #19 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #15)
> The following removes the premature optimization(?) using
> EDGE_NO_COPY_SRC_BLOCK
> in case the block contained a condition we could thread through.  That fixes
> the testcase.
> 
> diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
> index c3ea2d680d8..2005e30322b 100644
> --- a/gcc/tree-ssa-threadedge.c
> +++ b/gcc/tree-ssa-threadedge.c
> @@ -989,8 +989,11 @@ thread_around_empty_blocks (edge taken_edge,
>         return false;
>        bitmap_set_bit (visited, taken_edge->dest->index);
>  
> +      /* We may not use EDGE_NO_COPY_SRC_BLOCK since the condition
> +        might later be simplified to something needing a temporary
> +        SSA definition.  See PR102844.  */
>        jump_thread_edge *x
> -       = new jump_thread_edge (taken_edge, EDGE_NO_COPY_SRC_BLOCK);
> +       = new jump_thread_edge (taken_edge, EDGE_COPY_SRC_BLOCK);
>        path->safe_push (x);
>  
>        thread_around_empty_blocks (taken_edge,

OK, it isn't that easy - we overrun redirection_data::dup_blocks that way
causing random crashes later on.  So the change needs adjustments to how
we account thread_around_empty_blocks stuff, I couldn't find the code
that avoids this for non-empty blocks.

The issue is definitely latent everywhere even trunk as long as we keep
the DOM forward threader there.

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #20 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Aldy Hernandez from comment #18)
> (In reply to rguenther@suse.de from comment #17)
> > On Wed, 20 Oct 2021, aldyh at gcc dot gnu.org wrote:
> 
> > > Silly question, why is the SSA form invalid on entry to VRP2?  That's just
> > > asking for trouble.  Is this related to how asserts work?
> > 
> > Well, DOM threading creates invalid SSA (definition not dominating use).
> > Doesn't have to do anything with VRP or asserts.
> 
> Ah, I see.
> 
> BTW, if this is still the case in mainline, this is bound to be a problem
> for the ranger.  Andrew, won't we get an UNDEFINED / unreachable if we query
> the non dominating use at this point?

Well, invalid IL is invalid - there's no need to "cope" with it.  We have to
fix the (forward) threading code.

[Bug tree-optimization/102844] [9/10/11/12 Regression] vrp inserting assert causes miscompiling when doing update_ssa

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #21 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #12)
> (In reply to Martin Liška from comment #10)
> > (In reply to Richard Biener from comment #9)
> > > (In reply to Martin Liška from comment #8)
> > > > Using x86_64 compiler, the issue is fixed on master after
> > > > r10-1420-g744fd446c321f78f and started with r9-6384-g1a438d160e1dc845.
> > > 
> > > Can you continue searching for a fix with -fdisable-tree-fre4 (r10-1420
> > > added FRE4 before DOM3).
> > 
> > Sure, then it gets fixed with r11-408-g84935c9822183ce4.
> 
> -fno-tree-sink?

This is fixed with r11-4637-gf5e18dd9c7dacc96.

[Bug tree-optimization/102844] [9/10/11/12 Regression] DOM jump threading not copying block that became non-empty

[amacleod at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #22 from Andrew Macleod <amacleod at redhat dot com> ---
(In reply to Richard Biener from comment #20)
> (In reply to Aldy Hernandez from comment #18)
> > (In reply to rguenther@suse.de from comment #17)
> > > On Wed, 20 Oct 2021, aldyh at gcc dot gnu.org wrote:
> > 
> > > > Silly question, why is the SSA form invalid on entry to VRP2?  That's just
> > > > asking for trouble.  Is this related to how asserts work?
> > > 
> > > Well, DOM threading creates invalid SSA (definition not dominating use).
> > > Doesn't have to do anything with VRP or asserts.
> > 
> > Ah, I see.
> > 
> > BTW, if this is still the case in mainline, this is bound to be a problem
> > for the ranger.  Andrew, won't we get an UNDEFINED / unreachable if we query
> > the non dominating use at this point?
> 
> Well, invalid IL is invalid - there's no need to "cope" with it.  We have to
> fix the (forward) threading code.

Indeed, invalid is invalid.

And it depends. Any on-entry range is the union of all incoming blocks. If we
query a use that has no def anywhere in the dominator tree , then you will get
UNDEFINED. If the def is on at least one incoming path, then you will get the
def or some derivative of it.

[Bug tree-optimization/102844] [9/10/11/12 Regression] DOM jump threading not copying block that became non-empty

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #23 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Invalid is invalid.  Full stop.

I'll have to put it under a debugger, but I would have expected the nocopy
block to turn into a forwarder -- why do we end up putting statements in here?

[Bug tree-optimization/102844] [9/10/11/12 Regression] DOM jump threading not copying block that became non-empty

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #24 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jeffrey A. Law from comment #23)
> Invalid is invalid.  Full stop.
> 
> I'll have to put it under a debugger, but I would have expected the nocopy
> block to turn into a forwarder -- why do we end up putting statements in
> here?

We are folding the GIMPLE_COND as part of DOMs stmt optimization (we
propagate a copy into it), and the simplification gets us a conversion
we need to put somewhere.

Before r9-6384-g1a438d160e1dc845 we were failing to put such new
stmts into the expression hash tables leading to redundancies.

[Bug tree-optimization/102844] [9/10/11/12/13 Regression] DOM jump threading not copying block that became non-empty

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #25 from Richard Biener <rguenth at gcc dot gnu.org> ---
I'm not sure about the state of this bug - the issue reproduces on the GCC 10
branch with checking enabled and -O[2s] -fdisable-tree-fre4
-fno-strict-overflow

It might be that using the backward threader from DOM (are we now doing that?)
makes this truly fixed but I'm not sure the backward threader is resilent
to the issue analyzed in comments #13 to #15.  Eventually the NOCOPY
special-casing could be removed as premature optimization (CFG cleanup will
remove any copied forwarders), but then IIRC we have a hard limit on the
number of blocks the threader handles.

[Bug tree-optimization/102844] [9/10/11/12/13 Regression] DOM jump threading not copying block that became non-empty

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #26 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #25)
> I'm not sure about the state of this bug - the issue reproduces on the GCC
> 10 branch with checking enabled and -O[2s] -fdisable-tree-fre4
> -fno-strict-overflow
> 
> It might be that using the backward threader from DOM (are we now doing
> that?)
> makes this truly fixed but I'm not sure the backward threader is resilent
> to the issue analyzed in comments #13 to #15.  Eventually the NOCOPY

No.  DOM is the only remaining use of the forward threader.  It does not use
the backward threader, nor the ranger path solver.

[Bug tree-optimization/102844] [9/10/11/12/13 Regression] DOM jump threading not copying block that became non-empty

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #27 from Jeffrey A. Law <law at gcc dot gnu.org> ---
We're not using the backward threader to replace DOM's threader yet.  I've got
a TODO to push on Aldy's patch, but haven't been able to get to it over the
last couple weeks.  It's an, umm, busy time for us right now.

There's really only one issue we need to hash out in Aldy's work and there's a
couple proposed workarounds/interm solutions on our way to removing both the
forward threader and DOM itself.

[Bug tree-optimization/102844] [9/10/11/12/13 Regression] DOM jump threading not copying block that became non-empty

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #28 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to Jeffrey A. Law from comment #27)
> We're not using the backward threader to replace DOM's threader yet.  I've
> got a TODO to push on Aldy's patch, but haven't been able to get to it over
> the last couple weeks.  It's an, umm, busy time for us right now.
> 
> There's really only one issue we need to hash out in Aldy's work and there's
> a couple proposed workarounds/interm solutions on our way to removing both
> the forward threader and DOM itself.

And even that patch does not replace the forward threader.  It is a hybrid
approach using the forward threader, but the ranger path solver (instead of
evrp).  The forward threader should be with us until ranger can do floats and
pointers later this season.

[Bug tree-optimization/102844] [10/11/12/13 Regression] DOM jump threading not copying block that became non-empty

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.5                         |10.4

--- Comment #29 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 9 branch is being closed

[Bug tree-optimization/102844] [10/11/12/13 Regression] DOM jump threading not copying block that became non-empty

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #30 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug tree-optimization/102844] [10/11/12/13 Regression] DOM jump threading not copying block that became non-empty

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

--- Comment #31 from Richard Biener <rguenth at gcc dot gnu.org> ---
The issue is still latent but cannot be reproduced on any maintained branch.

[Bug tree-optimization/102844] [11/12/13/14 Regression] DOM jump threading not copying block that became non-empty

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5

--- Comment #32 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10 branch is being closed.

[Bug tree-optimization/102844] [11/12/13/14 Regression] DOM jump threading not copying block that became non-empty

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |seurer at gcc dot gnu.org

--- Comment #33 from Richard Biener <rguenth at gcc dot gnu.org> ---
*** Bug 111465 has been marked as a duplicate of this bug. ***

[Bug tree-optimization/102844] [11/12/13 Regression] DOM jump threading not copying block that became non-empty

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102844

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |14.0
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
             Status|NEW                         |ASSIGNED
            Summary|[11/12/13/14 Regression]    |[11/12/13 Regression] DOM
                   |DOM jump threading not      |jump threading not copying
                   |copying block that became   |block that became non-empty
                   |non-empty                   |

--- Comment #34 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed by r14-4128-g564ecb7d5afb0b.  Mine for backporting.