This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][debug] Fix pre_dec handling in vartrack
- From: Jakub Jelinek <jakub at redhat dot com>
- To: Tom de Vries <tdevries at suse dot de>
- Cc: gcc-patches at gcc dot gnu dot org, Jim Wilson <wilson at tuliptree dot org>, Jason Merrill <jason at redhat dot com>, Cary Coutant <ccoutant at gmail dot com>
- Date: Mon, 16 Jul 2018 11:02:06 +0200
- Subject: Re: [PATCH][debug] Fix pre_dec handling in vartrack
- References: <20180715212156.z2fjckukdui5ackf@delia> <20180716072410.GY7166@tucnak>
- Reply-to: Jakub Jelinek <jakub at redhat dot com>
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