[Bug tree-optimization/100382] New: [12 Regression] go.test/test/fixedbugs/issue16095.go hang since r12-248

[Bug tree-optimization/100382] New: [12 Regression] go.test/test/fixedbugs/issue16095.go hang since r12-248

[jakub at gcc dot gnu.org]

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

            Bug ID: 100382
           Summary: [12 Regression] go.test/test/fixedbugs/issue16095.go
                    hang since r12-248
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jakub at gcc dot gnu.org
  Target Milestone: ---

Since r12-248-gb58dc0b803057c0e6032e0d9bd92cd834f72c75c the
go.test/test/fixedbugs/issue16095.go
test hangs.
To reproduce e.g.:
../configure --enable-languages=c,c++,go --disable-bootstrap
make -j32
cd gcc
make -j32 check-go
The first difference (good to bad) is in the dse2 dump:
@@ -889,7 +889,6 @@ void main.main ()
   uint8[20] * _69;
   uint32 _72;
   uint64 GOTMP.131_73;
-  int _74;
   bool GOTMP.132_75;
   bool GOTMP.132_77;
   int GOTMP.16_155;
@@ -1190,7 +1189,6 @@ void main.main ()
   goto <bb 52>; [100.00%]

   <bb 54> [count: 0]:
-  _74 = MEM[(int *)0B];
   goto <bb 54>; [100.00%]

   <bb 55> [count: 0]:
@@ -1217,7 +1215,7 @@ void main.main ()
   goto <bb 68>; [100.00%]

   <bb 59> [count: 0]:
-  # x_76 = PHI <x_78(D)(36), _61(37), _61(41), _61(44), _61(45), _61(46),
_61(47), _61(48), _61(53), _61(54)>
+  # x_76 = PHI <x_78(D)(36), _61(37), _61(41), _61(44), _61(45), _61(46),
_61(47), _61(48), _61(53)>
 <L28>:
   # DEBUG x => x_76
   runtime.checkdefer (&GOTMP.15);

[Bug tree-optimization/100382] [12 Regression] go.test/test/fixedbugs/issue16095.go hang since r12-248

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ian at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org
   Target Milestone|---                         |12.0

[Bug tree-optimization/100382] [12 Regression] go.test/test/fixedbugs/issue16095.go hang since r12-248

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-05-02

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 50736
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50736&action=edit
gcc12-pr100382.patch

So perhaps this untested fix?

[Bug tree-optimization/100382] [12 Regression] go.test/test/fixedbugs/issue16095.go hang since r12-248

[rguenth at gcc dot gnu.org]

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

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Hum.  OK, so DSE would also miss is_ctrl_altering_stmt (DCE checks this which
covers stmt_can_throw_internal).  So with non-call-EH we even want to preserve
externally throwing EH?  Given there's can_delete_dead_exceptions I wonder
if this applies to call EH as well.  Note for this to make a practical
difference the throwing function would need to be pure at least (thus subject
to DCE in the first place).

So - wouldn't a more consistent check be

    (!stmt_could_throw_p ()
     || cfun->can_delete_dead_exceptions)

?

int x, y;
int __attribute__((pure,noinline)) foo () { if (x) throw; return y; }

int main()
{
  int a[2];
  x = 1;
  try {
    int res = foo ();
    a[0] = res;
  } catch (...) {
      return 0;
  }
  return 1;
}

should show this but the post-dom domwalk is plagued by the same ordering
issue as the dom domwalk was (we visit dom children in non-"RPO" order,
in this case inverted postorder).

[Bug tree-optimization/100382] [12 Regression] go.test/test/fixedbugs/issue16095.go hang since r12-248

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
With the following patch this "issue" would show.  Not sure why we think
a postdom walk is appropriate for DSE rather than a reverse program order one.

diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
index aecf6ab8c46..5bb5adf43c6 100644
--- a/gcc/tree-ssa-dse.c
+++ b/gcc/tree-ssa-dse.c
@@ -39,6 +39,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "builtins.h"
 #include "gimple-fold.h"
 #include "gimplify.h"
+#include "cfganal.h"

 /* This file implements dead store elimination.

@@ -1280,7 +1281,16 @@ pass_dse::execute (function *fun)
   /* Dead store elimination is fundamentally a walk of the post-dominator
      tree and a backwards walk of statements within each block.  */
   dse_dom_walker walker (CDI_POST_DOMINATORS);
-  walker.walk (fun->cfg->x_exit_block_ptr);
+  //walker.walk (fun->cfg->x_exit_block_ptr);
+  int *rpo = XNEWVEC (int, n_basic_blocks_for_fn (fun) - NUM_FIXED_BLOCKS);
+  auto_bitmap exits;
+  edge entry = single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (fun));
+  bitmap_set_bit (exits, EXIT_BLOCK);
+  int n = rev_post_order_and_mark_dfs_back_seme
+    (fun, entry, exits, true, rpo, NULL);
+  for (int i = n; i != 0; --i)
+    walker.before_dom_children (BASIC_BLOCK_FOR_FN (fun, rpo[i-1]));
+  free (rpo);
   free_dominance_info (CDI_POST_DOMINATORS);

   unsigned todo = walker.todo ();

[Bug tree-optimization/100382] [12 Regression] go.test/test/fixedbugs/issue16095.go hang since r12-248

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:80bbb7ff83d81544b09820428bdd7db9f50fe362

commit r12-378-g80bbb7ff83d81544b09820428bdd7db9f50fe362
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon May 3 12:03:02 2021 +0200

    tree-ssa-dse: Fix up go.test/test/fixedbugs/issue16095.go miscompilation
[PR100382]

    The new DCE code inside of tree DSE removes in -fnon-call-exceptions
    go code a load from NULL pointer the testcase relies on throwing an
    exception and so the test hangs.

    The following patch just repeats a check that e.g. tree-ssa-dce.c
    uses to prevent this.

    2021-05-03  Jakub Jelinek  <jakub@redhat.com>

            PR tree-optimization/100382
            * tree-ssa-dse.c: Include tree-eh.h.
            (dse_dom_walker::before_dom_children): Don't remove stmts if
            stmt_unremovable_because_of_non_call_eh_p is true.

[Bug tree-optimization/100382] [12 Regression] go.test/test/fixedbugs/issue16095.go hang since r12-248

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.

[Bug tree-optimization/100382] [12 Regression] go.test/test/fixedbugs/issue16095.go hang since r12-248

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Andrew Pinski <pinskia@gcc.gnu.org>:

https://gcc.gnu.org/g:7518e4c2f0758daac5d650d400565cf49ac3c8c5

commit r12-4661-g7518e4c2f0758daac5d650d400565cf49ac3c8c5
Author: Andrew Pinski <apinski@marvell.com>
Date:   Sat Oct 23 19:24:43 2021 +0000

    Fix PR 102908: wrongly removing null pointer loads

    Just like PR 100382, here we have a DCE removing a
    null pointer load which is needed still.
    In this case, execute_fixup_cfg removes a store (correctly)
    and then removes the null load (incorrectly) due to
    not checking stmt_unremovable_because_of_non_call_eh_p.
    This patch adds the check in the similar way as the patch
    to fix PR 100382 did.

    gcc/ChangeLog:

            * tree-ssa-dce.c (simple_dce_from_worklist):
            Check stmt_unremovable_because_of_non_call_eh_p also
            before removing the statement.