Bug 86638 - Og guality failures without -ftree-sra
Summary: Og guality failures without -ftree-sra
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: debug (show other bugs)
Version: 9.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks: ohgee
  Show dependency treegraph
 
Reported: 2018-07-23 07:40 UTC by Tom de Vries
Modified: 2019-08-05 09:35 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-07-23 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom de Vries 2018-07-23 07:40:36 UTC
There's a class of guality failures at Og that goes away when using -ftree-sra:
...
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+4 a[0] == 1
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+3 a[1] == 2
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+2 a[2] == 3
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+1 *p == 3
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line . *q == 2
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+4 a[0] == 1
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+3 a[1] == 2
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+2 a[2] == 13
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+1 *p == 13
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line . *q == 2
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+4 a[0] == 1
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+3 a[1] == 12
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+2 a[2] == 13
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+1 *p == 13
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line . *q == 12
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+3 a[1] == 5
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+2 a[2] == 6
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+1 *p == 6
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line . *q == 5
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+3 a[1] == 5
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+2 a[2] == 26
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+1 *p == 26
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line . *q == 5
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+7 a[1] == 25
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+6 a[2] == 26
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+5 *p == 26
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+4 p[-1] == 25
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line .+1 q[1] == 26
FAIL: gcc.dg/guality/pr54970.c  -Og -DPREVENT_OPTIMIZATION  line . *q == 25

FAIL: gcc.dg/guality/pr56154-1.c  -Og -DPREVENT_OPTIMIZATION  line pr56154-1.c:20 x.a == 6

FAIL: gcc.dg/guality/pr59776.c  -Og -DPREVENT_OPTIMIZATION  line pr59776.c:17 s1.f == 5.0
FAIL: gcc.dg/guality/pr59776.c  -Og -DPREVENT_OPTIMIZATION  line pr59776.c:17 s1.g == 6.0
FAIL: gcc.dg/guality/pr59776.c  -Og -DPREVENT_OPTIMIZATION  line pr59776.c:17 s2.f == 0.0
FAIL: gcc.dg/guality/pr59776.c  -Og -DPREVENT_OPTIMIZATION  line pr59776.c:17 s2.g == 6.0
FAIL: gcc.dg/guality/pr59776.c  -Og -DPREVENT_OPTIMIZATION  line pr59776.c:20 s1.f == 5.0
FAIL: gcc.dg/guality/pr59776.c  -Og -DPREVENT_OPTIMIZATION  line pr59776.c:20 s1.g == 6.0
FAIL: gcc.dg/guality/pr59776.c  -Og -DPREVENT_OPTIMIZATION  line pr59776.c:20 s2.f == 5.0
FAIL: gcc.dg/guality/pr59776.c  -Og -DPREVENT_OPTIMIZATION  line pr59776.c:20 s2.g == 6.0
...
Comment 1 Tom de Vries 2018-07-23 09:02:14 UTC
F.i., take pr56154-1.c:
...
     1  /* PR debug/56154 */
     2  /* { dg-do run } */
     3  /* { dg-options "-g" } */
     4  /* { dg-additional-sources "pr56154-aux.c" } */
     5
     6  #include "../nop.h"
     7
     8  union U { int a, b; };
     9  volatile int z;
    10
    11  __attribute__((noinline, noclone)) int
    12  foo (int fd, union U x)
    13  {
    14    int result = x.a != 0;
    15    if (fd != 0)
    16      result = x.a == 0;
    17    asm (NOP : : : "memory");  /* { dg-final { gdb-test pr56154-1.c:17 "x.a" "4" } } */
    18    z = x.a;
    19    x.a = 6;
    20    asm (NOP : : : "memory");  /* { dg-final { gdb-test pr56154-1.c:20 "x.a" "6" } } */
    21    return result;
    22  }
    23
    24  void
    25  test_main (void)
    26  {
    27    union U u = { .a = 4 };
    28    foo (0, u);
    29  }
...

which fails like this:
...
FAIL: gcc.dg/guality/pr56154-1.c -Og -DPREVENT_OPTIMIZATION  line pr56154-1.c:20 x.a == 6
...

Without -ftree-sra, we have:
...
$ grep DEBUG pr56154-1.c.228t.optimized | grep -v BEGIN_STMT
  # DEBUG result => result_7
  # DEBUG result => result_9
  # DEBUG result => result_5
...

and with -ftree-sra, we have:
...
$ grep DEBUG pr56154-1.c.228t.optimized | grep -v BEGIN_STMT
  # DEBUG x$a => x$a_11
  # DEBUG result => result_5
  # DEBUG result => result_7
  # DEBUG result => result_3
  # DEBUG x$a => 6
...

In general, we might be able to improve the situation by emitting var_location at expand for non-ssa vars that we emit in registers.

But in this case it won't help us, because the store of 6 to x.a is already removed by dce by the time we arrive at expand.

Using the fkeep-vars-live patch, we manage to prevent the dce, and are able to print the '6' value of x one line later, at line 21, but not at line 20, due to a "DEBUG x RESET".

AFAIU, the var-tracking manages to deduce from the artificial use inserted by fkeep-vars-live that x is in reg si at ret, but it can't deduce that the store of 6 into reg si is also related to x.
...
        .loc 1 19 3 is_stmt 1
        .loc 1 19 7 is_stmt 0
        movl    $6, %esi
.LVL4:
        # DEBUG x RESET
        .loc 1 20 3 is_stmt 1
        nop
        .loc 1 21 3
.LVL5:
        # DEBUG x => si
        .loc 1 22 1 is_stmt 0
        ret
...
But, if we'd insert the var_location of x at expand (maybe after every assign to x), we could deduce that the store of 6 is related to x, and we'd be able to print the value of x at line 20.
Comment 2 Richard Biener 2018-07-23 09:16:06 UTC
Hmm, it sounds like DCE/DSE should insert

# DEBUG x$a => x$a_11

kind debug stmts.  IIRC SRA does more than that, adding DECL_DEBUG_EXPRs
with magic.

Not sure if the debug stmts itself help enough here.
Comment 3 Tom de Vries 2018-07-23 10:02:36 UTC
(In reply to Richard Biener from comment #2)
> Hmm, it sounds like DCE/DSE should insert
> 
> # DEBUG x$a => x$a_11
> 
> kind debug stmts.  IIRC SRA does more than that, adding DECL_DEBUG_EXPRs
> with magic.
> 
> Not sure if the debug stmts itself help enough here.

At cddce1, we have:
...
 __attribute__((noclone, noinline))
 foo (int fd, union U x)
 {
   int result;
   int _1;
   _Bool _2;
   _Bool _4;
   int _5;
 
   <bb 2> :
   # DEBUG BEGIN_STMT
   _1 = x.a;
   _2 = _1 != 0;
   result_8 = (int) _2;
   # DEBUG result => result_8
   # DEBUG BEGIN_STMT
   if (fd_9(D) != 0)
     goto <bb 3>; [INV]
   else
     goto <bb 4>; [INV]
 
   <bb 3> :
   # DEBUG BEGIN_STMT
   _4 = _1 == 0;
   result_10 = (int) _4;
   # DEBUG result => result_10
 
   <bb 4> :
   # result_6 = PHI <result_8(2), result_10(3)>
   # DEBUG result => result_6
   # DEBUG BEGIN_STMT
   __asm__ __volatile__("nop" :  :  : "memory");
   # DEBUG BEGIN_STMT
   _5 = x.a;
   z ={v} _5;
   # DEBUG BEGIN_STMT
-  x.a = 6;
   # DEBUG BEGIN_STMT
   __asm__ __volatile__("nop" :  :  : "memory");
   # DEBUG BEGIN_STMT
   return result_6;
 
 }
...

So, are you proposing to keep track of components like this:
...
-  x.a = 6;
+  # DEBUG x.a => 6
...
?
Comment 4 Richard Sandiford 2019-07-29 08:53:27 UTC
Author: rsandifo
Date: Mon Jul 29 08:52:56 2019
New Revision: 273872

URL: https://gcc.gnu.org/viewcvs?rev=273872&root=gcc&view=rev
Log:
Prevent tree-ssa-dce.c from deleting stores at -Og

DCE tries to delete dead stores to local data and also tries to insert
debug binds for simple cases:

  /* If this is a store into a variable that is being optimized away,
     add a debug bind stmt if possible.  */
  if (MAY_HAVE_DEBUG_BIND_STMTS
      && gimple_assign_single_p (stmt)
      && is_gimple_val (gimple_assign_rhs1 (stmt)))
    {
      tree lhs = gimple_assign_lhs (stmt);
      if ((VAR_P (lhs) || TREE_CODE (lhs) == PARM_DECL)
	  && !DECL_IGNORED_P (lhs)
	  && is_gimple_reg_type (TREE_TYPE (lhs))
	  && !is_global_var (lhs)
	  && !DECL_HAS_VALUE_EXPR_P (lhs))
	{
	  tree rhs = gimple_assign_rhs1 (stmt);
	  gdebug *note
	    = gimple_build_debug_bind (lhs, unshare_expr (rhs), stmt);
	  gsi_insert_after (i, note, GSI_SAME_STMT);
	}
    }

But this doesn't help for things like "print *ptr" when ptr points
to the local variable (tests Og-dce-1.c and Og-dce-2.c).  It can
also introduce wrong debug info for earlier references (second test
in Og-dce-3.c) or make earlier references unavailable (first test
in Og-dce-3.c).

So for -Og I think it'd be better not to delete any stmts with
vdefs for now.  This also means that we can avoid the potentially
expensive vop walks (which already have a cut-off, but still).

The patch also fixes the Og failures in gcc.dg/guality/pr54970.c
(PR 86638).

2019-07-29  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	PR debug/86638
	* tree-ssa-dce.c (keep_all_vdefs_p): New function.
	(mark_stmt_if_obviously_necessary): Mark all stmts with vdefs as
	necessary if keep_all_vdefs_p is true.
	(mark_aliased_reaching_defs_necessary): Add a gcc_checking_assert
	that keep_all_vdefs_p is false.
	(mark_all_reaching_defs_necessary): Likewise.
	(propagate_necessity): Skip the vuse scan if keep_all_vdefs_p is true.

gcc/testsuite/
	* c-c++-common/guality/Og-dce-1.c: New test.
	* c-c++-common/guality/Og-dce-2.c: Likewise.
	* c-c++-common/guality/Og-dce-3.c: Likewise.

Added:
    trunk/gcc/testsuite/c-c++-common/guality/Og-dce-1.c
    trunk/gcc/testsuite/c-c++-common/guality/Og-dce-2.c
    trunk/gcc/testsuite/c-c++-common/guality/Og-dce-3.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-dce.c
Comment 5 Eric Gallager 2019-08-04 15:55:25 UTC
(In reply to rsandifo@gcc.gnu.org from comment #4)
> Author: rsandifo
> Date: Mon Jul 29 08:52:56 2019
> New Revision: 273872
> 
> URL: https://gcc.gnu.org/viewcvs?rev=273872&root=gcc&view=rev
> Log:
> Prevent tree-ssa-dce.c from deleting stores at -Og
> 
> DCE tries to delete dead stores to local data and also tries to insert
> debug binds for simple cases:
> 
>   /* If this is a store into a variable that is being optimized away,
>      add a debug bind stmt if possible.  */
>   if (MAY_HAVE_DEBUG_BIND_STMTS
>       && gimple_assign_single_p (stmt)
>       && is_gimple_val (gimple_assign_rhs1 (stmt)))
>     {
>       tree lhs = gimple_assign_lhs (stmt);
>       if ((VAR_P (lhs) || TREE_CODE (lhs) == PARM_DECL)
> 	  && !DECL_IGNORED_P (lhs)
> 	  && is_gimple_reg_type (TREE_TYPE (lhs))
> 	  && !is_global_var (lhs)
> 	  && !DECL_HAS_VALUE_EXPR_P (lhs))
> 	{
> 	  tree rhs = gimple_assign_rhs1 (stmt);
> 	  gdebug *note
> 	    = gimple_build_debug_bind (lhs, unshare_expr (rhs), stmt);
> 	  gsi_insert_after (i, note, GSI_SAME_STMT);
> 	}
>     }
> 
> But this doesn't help for things like "print *ptr" when ptr points
> to the local variable (tests Og-dce-1.c and Og-dce-2.c).  It can
> also introduce wrong debug info for earlier references (second test
> in Og-dce-3.c) or make earlier references unavailable (first test
> in Og-dce-3.c).
> 
> So for -Og I think it'd be better not to delete any stmts with
> vdefs for now.  This also means that we can avoid the potentially
> expensive vop walks (which already have a cut-off, but still).
> 
> The patch also fixes the Og failures in gcc.dg/guality/pr54970.c
> (PR 86638).
> 

So can this be closed now then?

> 2019-07-29  Richard Sandiford  <richard.sandiford@arm.com>
> 
> gcc/
> 	PR debug/86638
> 	* tree-ssa-dce.c (keep_all_vdefs_p): New function.
> 	(mark_stmt_if_obviously_necessary): Mark all stmts with vdefs as
> 	necessary if keep_all_vdefs_p is true.
> 	(mark_aliased_reaching_defs_necessary): Add a gcc_checking_assert
> 	that keep_all_vdefs_p is false.
> 	(mark_all_reaching_defs_necessary): Likewise.
> 	(propagate_necessity): Skip the vuse scan if keep_all_vdefs_p is true.
> 
> gcc/testsuite/
> 	* c-c++-common/guality/Og-dce-1.c: New test.
> 	* c-c++-common/guality/Og-dce-2.c: Likewise.
> 	* c-c++-common/guality/Og-dce-3.c: Likewise.
> 
> Added:
>     trunk/gcc/testsuite/c-c++-common/guality/Og-dce-1.c
>     trunk/gcc/testsuite/c-c++-common/guality/Og-dce-2.c
>     trunk/gcc/testsuite/c-c++-common/guality/Og-dce-3.c
> Modified:
>     trunk/gcc/ChangeLog
>     trunk/gcc/testsuite/ChangeLog
>     trunk/gcc/tree-ssa-dce.c
Comment 6 Richard Sandiford 2019-08-05 09:35:36 UTC
Fixed on trunk (a while ago... thanks Eric for the reminder :-))