[debug] Fix pre_dec handling in vartrack

Message ID 20180715212156.z2fjckukdui5ackf@delia
State New
Headers show
Series
  • [debug] Fix pre_dec handling in vartrack
Related show

Commit Message

Tom de Vries July 15, 2018, 9:21 p.m.
Hi,

when compiling test-case gcc.target/i386/vartrack-1.c with -O1 -g, register bx
is pushed in the prologue and popped in the epilogue:
...
(insn/f 26 3 27 2
  (set (mem:DI (pre_dec:DI (reg/f:DI 7 sp)) [0  S8 A8])
       (reg:DI 3 bx))
   "vartrack-1.c":10 61 {*pushdi2_rex64}
   (expr_list:REG_DEAD (reg:DI 3 bx) (nil)))
  ...
(insn/f 29 28 30 2
  (set (reg:DI 3 bx)
       (mem:DI (post_inc:DI (reg/f:DI 7 sp)) [0  S8 A8]))
   "vartrack-1.c":15 71 {*popdi1}
   (expr_list:REG_CFA_ADJUST_CFA
     (set (reg/f:DI 7 sp)
          (plus:DI (reg/f:DI 7 sp)
                   (const_int 8 [0x8]))) (nil)))
...

However, when we adjust those insns in vartrack to eliminate the pre_dec and
post_inc, the frame location for the push is at argp - 24, while the one for the
pop is at argp - 16:
...
(insn/f 26 3 27 2
  (parallel [
    (set (mem:DI (plus:DI (reg/f:DI 16 argp)
                          (const_int -24 [0xffffffffffffffe8])) [0  S8 A8])
         (reg:DI 3 bx))
    (set (reg/f:DI 7 sp)
         (plus:DI (reg/f:DI 16 argp)
                  (const_int -24 [0xffffffffffffffe8])))
  ])
  "vartrack-1.c":10 61 {*pushdi2_rex64}
  (expr_list:REG_DEAD (reg:DI 3 bx) (nil)))
  ...
(insn/f 29 28 30 2
  (parallel [
    (set (reg:DI 3 bx)
         (mem:DI (plus:DI (reg/f:DI 16 argp)
                          (const_int -16 [0xfffffffffffffff0])) [0  S8 A8]))
    (set (reg/f:DI 7 sp)
         (plus:DI (reg/f:DI 16 argp)
                  (const_int -8 [0xfffffffffffffff8])))
  ])
  "vartrack-1.c":15 71 {*popdi1}
  (expr_list:REG_CFA_ADJUST_CFA
    (set (reg/f:DI 7 sp)
         (plus:DI (reg/f:DI 7 sp)
                  (const_int 8 [0x8]))) (nil)))
...

This patch fixes that by moving the stack_adjust modification after
adjust_insn in vt_initialize.  Also, the patch prints the adjusted insn in a
slim way if dump_flags contain TDF_SLIM, which makes the scan-rtl-dump in the
testcase easier.

Bootstrapped and reg-tested on x86_64.

OK for trunk?

Thanks,
- Tom

[debug] Fix pre_dec handling in vartrack

2018-07-15  Tom de Vries  <tdevries@suse.de>

	* var-tracking.c (vt_initialize): Fix pre_dec handling.  Print adjusted
	insn slim if dump_flags request TDF_SLIM.

	* gcc.target/i386/vartrack-1.c: New test.

---
 gcc/testsuite/gcc.target/i386/vartrack-1.c | 28 ++++++++++++++++++++++++++++
 gcc/var-tracking.c                         | 13 +++++++++++--
 2 files changed, 39 insertions(+), 2 deletions(-)

Comments

Jakub Jelinek July 16, 2018, 7:24 a.m. | #1
On Sun, Jul 15, 2018 at 11:21:56PM +0200, Tom de Vries wrote:
> 2018-07-15  Tom de Vries  <tdevries@suse.de>

> 

> 	* var-tracking.c (vt_initialize): Fix pre_dec handling.  Print adjusted

> 	insn slim if dump_flags request TDF_SLIM.

> 

> 	* gcc.target/i386/vartrack-1.c: New test.

> 

> ---

> --- a/gcc/var-tracking.c

> +++ b/gcc/var-tracking.c

> @@ -115,6 +115,7 @@

>  #include "tree-pretty-print.h"

>  #include "rtl-iter.h"

>  #include "fibonacci_heap.h"

> +#include "print-rtl.h"

>  

>  typedef fibonacci_heap <long, basic_block_def> bb_heap_t;

>  typedef fibonacci_node <long, basic_block_def> bb_heap_node_t;

> @@ -10208,12 +10209,17 @@ vt_initialize (void)

>  			    log_op_type (PATTERN (insn), bb, insn,

>  					 MO_ADJUST, dump_file);

>  			  VTI (bb)->mos.safe_push (mo);

> -			  VTI (bb)->out.stack_adjust += pre;

>  			}

>  		    }

>  

>  		  cselib_hook_called = false;

>  		  adjust_insn (bb, insn);

> +

> +		  if (!frame_pointer_needed)

> +		    {

> +		      if (pre)

> +			VTI (bb)->out.stack_adjust += pre;

> +		    }


That is certainly unexpected.  For the pre side-effects, they should be
applied before adjusting the insn, not after that.
I'll want to see this under the debugger.

>  		  if (DEBUG_MARKER_INSN_P (insn))

>  		    {

>  		      reemit_marker_as_note (insn);

> @@ -10227,7 +10233,10 @@ vt_initialize (void)

>  		      cselib_process_insn (insn);

>  		      if (dump_file && (dump_flags & TDF_DETAILS))

>  			{

> -			  print_rtl_single (dump_file, insn);

> +			  if (dump_flags & TDF_SLIM)

> +			    dump_insn_slim (dump_file, insn);

> +			  else

> +			    print_rtl_single (dump_file, insn);

>  			  dump_cselib_table (dump_file);

>  			}

>  		    }


This part is certainly ok.

	Jakub
Jakub Jelinek July 16, 2018, 9:02 a.m. | #2
On Mon, Jul 16, 2018 at 09:24:10AM +0200, Jakub Jelinek wrote:
> On Sun, Jul 15, 2018 at 11:21:56PM +0200, Tom de Vries wrote:

> > 2018-07-15  Tom de Vries  <tdevries@suse.de>

> > 

> > 	* var-tracking.c (vt_initialize): Fix pre_dec handling.  Print adjusted

> > 	insn slim if dump_flags request TDF_SLIM.

> > 

> > 	* gcc.target/i386/vartrack-1.c: New test.

> > 

> > ---

> > --- a/gcc/var-tracking.c

> > +++ b/gcc/var-tracking.c

> > @@ -115,6 +115,7 @@

> >  #include "tree-pretty-print.h"

> >  #include "rtl-iter.h"

> >  #include "fibonacci_heap.h"

> > +#include "print-rtl.h"

> >  

> >  typedef fibonacci_heap <long, basic_block_def> bb_heap_t;

> >  typedef fibonacci_node <long, basic_block_def> bb_heap_node_t;

> > @@ -10208,12 +10209,17 @@ vt_initialize (void)

> >  			    log_op_type (PATTERN (insn), bb, insn,

> >  					 MO_ADJUST, dump_file);

> >  			  VTI (bb)->mos.safe_push (mo);

> > -			  VTI (bb)->out.stack_adjust += pre;

> >  			}

> >  		    }

> >  

> >  		  cselib_hook_called = false;

> >  		  adjust_insn (bb, insn);

> > +

> > +		  if (!frame_pointer_needed)

> > +		    {

> > +		      if (pre)

> > +			VTI (bb)->out.stack_adjust += pre;

> > +		    }

> 

> That is certainly unexpected.  For the pre side-effects, they should be

> applied before adjusting the insn, not after that.

> I'll want to see this under the debugger.


You're right, adjust_mems takes the PRE_INC/PRE_DEC/PRE_MODIFY into account
too, so by adjusting stack_adjust before adjust_insn the effects happen
twice:
    case PRE_INC:
    case PRE_DEC:
      size = GET_MODE_SIZE (amd->mem_mode);
      addr = plus_constant (GET_MODE (loc), XEXP (loc, 0),
                            GET_CODE (loc) == PRE_INC ? size : -size);
Unless we have instructions where we e.g. pre_dec sp on the lhs and
use some mem based on sp on the rhs, but I'd hope that should be invalid
RTL, because it is ambiguous what value would sp on the rhs have.

Please change the above patch to do:
 		  adjust_insn (bb, insn);
+
+		  if (!frame_pointer_needed && pre)
+		    VTI (bb)->out.stack_adjust += pre;

Ok for trunk with that change.

> 

> >  		  if (DEBUG_MARKER_INSN_P (insn))

> >  		    {

> >  		      reemit_marker_as_note (insn);

> > @@ -10227,7 +10233,10 @@ vt_initialize (void)

> >  		      cselib_process_insn (insn);

> >  		      if (dump_file && (dump_flags & TDF_DETAILS))

> >  			{

> > -			  print_rtl_single (dump_file, insn);

> > +			  if (dump_flags & TDF_SLIM)

> > +			    dump_insn_slim (dump_file, insn);

> > +			  else

> > +			    print_rtl_single (dump_file, insn);

> >  			  dump_cselib_table (dump_file);

> >  			}

> >  		    }

> 

> This part is certainly ok.


	Jakub
Rainer Orth July 17, 2018, 9:17 a.m. | #3
Hi Jakub,

> On Mon, Jul 16, 2018 at 09:24:10AM +0200, Jakub Jelinek wrote:

>> On Sun, Jul 15, 2018 at 11:21:56PM +0200, Tom de Vries wrote:

>> > 2018-07-15  Tom de Vries  <tdevries@suse.de>

>> > 

>> > 	* var-tracking.c (vt_initialize): Fix pre_dec handling.  Print adjusted

>> > 	insn slim if dump_flags request TDF_SLIM.

>> > 

>> > 	* gcc.target/i386/vartrack-1.c: New test.

>> > 

>> > ---

>> > --- a/gcc/var-tracking.c

>> > +++ b/gcc/var-tracking.c

>> > @@ -115,6 +115,7 @@

>> >  #include "tree-pretty-print.h"

>> >  #include "rtl-iter.h"

>> >  #include "fibonacci_heap.h"

>> > +#include "print-rtl.h"

>> >  

>> >  typedef fibonacci_heap <long, basic_block_def> bb_heap_t;

>> >  typedef fibonacci_node <long, basic_block_def> bb_heap_node_t;

>> > @@ -10208,12 +10209,17 @@ vt_initialize (void)

>> >  			    log_op_type (PATTERN (insn), bb, insn,

>> >  					 MO_ADJUST, dump_file);

>> >  			  VTI (bb)->mos.safe_push (mo);

>> > -			  VTI (bb)->out.stack_adjust += pre;

>> >  			}

>> >  		    }

>> >  

>> >  		  cselib_hook_called = false;

>> >  		  adjust_insn (bb, insn);

>> > +

>> > +		  if (!frame_pointer_needed)

>> > +		    {

>> > +		      if (pre)

>> > +			VTI (bb)->out.stack_adjust += pre;

>> > +		    }

>> 

>> That is certainly unexpected.  For the pre side-effects, they should be

>> applied before adjusting the insn, not after that.

>> I'll want to see this under the debugger.

>

> You're right, adjust_mems takes the PRE_INC/PRE_DEC/PRE_MODIFY into account

> too, so by adjusting stack_adjust before adjust_insn the effects happen

> twice:

>     case PRE_INC:

>     case PRE_DEC:

>       size = GET_MODE_SIZE (amd->mem_mode);

>       addr = plus_constant (GET_MODE (loc), XEXP (loc, 0),

>                             GET_CODE (loc) == PRE_INC ? size : -size);

> Unless we have instructions where we e.g. pre_dec sp on the lhs and

> use some mem based on sp on the rhs, but I'd hope that should be invalid

> RTL, because it is ambiguous what value would sp on the rhs have.

>

> Please change the above patch to do:

>  		  adjust_insn (bb, insn);

> +

> +		  if (!frame_pointer_needed && pre)

> +		    VTI (bb)->out.stack_adjust += pre;

>

> Ok for trunk with that change.


it turns out the test FAILs on i386-pc-soalris2.11 with -m64: the dump
doesn't even contain argp:  However, adding -fomit-frame-pointer makes
it work, and still PASSes on x86_64-pc-linux-gnu.

Ok for for mainline?

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


2018-07-17  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	* gcc.target/i386/vartrack-1.c (dg-options): Add
	-fomit-frame-pointer.
diff --git a/gcc/testsuite/gcc.target/i386/vartrack-1.c b/gcc/testsuite/gcc.target/i386/vartrack-1.c
--- a/gcc/testsuite/gcc.target/i386/vartrack-1.c
+++ b/gcc/testsuite/gcc.target/i386/vartrack-1.c
@@ -1,5 +1,5 @@
 /* { dg-require-effective-target lp64 } */
-/* { dg-options "-O1 -g -fdump-rtl-vartrack-details-slim" } */
+/* { dg-options "-O1 -g -fomit-frame-pointer -fdump-rtl-vartrack-details-slim" } */
 
 static volatile int vv = 1;
Jakub Jelinek July 17, 2018, 9:22 a.m. | #4
On Tue, Jul 17, 2018 at 11:17:33AM +0200, Rainer Orth wrote:
> it turns out the test FAILs on i386-pc-soalris2.11 with -m64: the dump

> doesn't even contain argp:  However, adding -fomit-frame-pointer makes

> it work, and still PASSes on x86_64-pc-linux-gnu.

> 

> Ok for for mainline?


Ok, thanks.

> 2018-07-17  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

> 

> 	* gcc.target/i386/vartrack-1.c (dg-options): Add

> 	-fomit-frame-pointer.

> 


> diff --git a/gcc/testsuite/gcc.target/i386/vartrack-1.c b/gcc/testsuite/gcc.target/i386/vartrack-1.c

> --- a/gcc/testsuite/gcc.target/i386/vartrack-1.c

> +++ b/gcc/testsuite/gcc.target/i386/vartrack-1.c

> @@ -1,5 +1,5 @@

>  /* { dg-require-effective-target lp64 } */

> -/* { dg-options "-O1 -g -fdump-rtl-vartrack-details-slim" } */

> +/* { dg-options "-O1 -g -fomit-frame-pointer -fdump-rtl-vartrack-details-slim" } */

>  

>  static volatile int vv = 1;

>  



	Jakub

Patch

diff --git a/gcc/testsuite/gcc.target/i386/vartrack-1.c b/gcc/testsuite/gcc.target/i386/vartrack-1.c
new file mode 100644
index 00000000000..15514fc0da7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/vartrack-1.c
@@ -0,0 +1,28 @@ 
+/* { dg-require-effective-target lp64 } */
+/* { dg-options "-O1 -g -fdump-rtl-vartrack-details-slim" } */
+
+static volatile int vv = 1;
+
+extern long foo (long x);
+
+int
+main ()
+{
+  long x = vv;
+  foo (x);
+  foo (x + 1);
+  return 0;
+}
+
+/* Before adjust_insn:
+   26: [--sp:DI]=bx:DI
+   29: bx:DI=[sp:DI++]
+
+   after adjust_insn:
+   26: {[argp:DI-0x10]=bx:DI;sp:DI=argp:DI-0x10;}
+   29: {bx:DI=[argp:DI-0x10];sp:DI=argp:DI-0x8;} */
+
+/* { dg-final { scan-rtl-dump-times {[0-9][0-9]*: \{\[argp:DI-0x10\]=bx:DI;sp:DI=argp:DI-0x10;\}} 1 "vartrack" } } */
+
+/* { dg-final { scan-rtl-dump-times {[0-9][0-9]*: \{bx:DI=\[argp:DI-0x10\];sp:DI=argp:DI-0x8;\}} 1 "vartrack" } } */
+
diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
index 8e800960b6d..5058f3cc7d2 100644
--- a/gcc/var-tracking.c
+++ b/gcc/var-tracking.c
@@ -115,6 +115,7 @@ 
 #include "tree-pretty-print.h"
 #include "rtl-iter.h"
 #include "fibonacci_heap.h"
+#include "print-rtl.h"
 
 typedef fibonacci_heap <long, basic_block_def> bb_heap_t;
 typedef fibonacci_node <long, basic_block_def> bb_heap_node_t;
@@ -10208,12 +10209,17 @@  vt_initialize (void)
 			    log_op_type (PATTERN (insn), bb, insn,
 					 MO_ADJUST, dump_file);
 			  VTI (bb)->mos.safe_push (mo);
-			  VTI (bb)->out.stack_adjust += pre;
 			}
 		    }
 
 		  cselib_hook_called = false;
 		  adjust_insn (bb, insn);
+
+		  if (!frame_pointer_needed)
+		    {
+		      if (pre)
+			VTI (bb)->out.stack_adjust += pre;
+		    }
 		  if (DEBUG_MARKER_INSN_P (insn))
 		    {
 		      reemit_marker_as_note (insn);
@@ -10227,7 +10233,10 @@  vt_initialize (void)
 		      cselib_process_insn (insn);
 		      if (dump_file && (dump_flags & TDF_DETAILS))
 			{
-			  print_rtl_single (dump_file, insn);
+			  if (dump_flags & TDF_SLIM)
+			    dump_insn_slim (dump_file, insn);
+			  else
+			    print_rtl_single (dump_file, insn);
 			  dump_cselib_table (dump_file);
 			}
 		    }