Patchwork RFA: Fix debug-insn sensitivity in RA

login
register
mail settings
Submitter Richard Sandiford
Date Sept. 7, 2013, 9:14 a.m.
Message ID <87a9jpj8mq.fsf@talisman.default>
Download mbox | patch
Permalink /patch/273378/
State New
Headers show

Comments

Richard Sandiford - Sept. 7, 2013, 9:14 a.m.
While doing some work on wide-int, I hit a bootstrap comparison failure that
was caused by the RA output depending on debug insns.  There was a register R
that was used normally by all "real" insns, but which was used in a paradoxical
subreg by one of the debug insns.  There was also a natural equivalence
between R and a memory.  The problem was that the paradoxical subreg in
the debug insn was stopping that equivalence from being used.

The problem seems to be split across IRA and LRA.  In IRA we have:

  FOR_EACH_BB (bb)
    FOR_BB_INSNS (bb, insn)
      {
	if (! INSN_P (insn))
	  continue;
	for_each_rtx (&insn, set_paradoxical_subreg, (void *)pdx_subregs);
      }

which sets pdx_subregs[R] if R is used in a paradoxical subreg, followed by:

	  /* Don't set reg (if pdx_subregs[regno] == true) equivalent to a mem.  */
	  if (MEM_P (src) && pdx_subregs[regno])
	    {
	      note_stores (set, no_equiv, NULL);
	      continue;
	    }

I think this should only happen when a paradoxical subreg is seen
in a nondebug insn.  If we use the equivalence when a debug insn has
a paradoxical subreg, the subreg will be replaced with a MEM that might
overlap surrounding data, but I think it's a valid transformation when
no run-time access will take place.  The data in the defined part of
the MEM should still be correct.

Then in LRA we keep track of the biggest mode that is used to refer to each
register (including references via paradoxical subregs).  This is done when
recording register references in an instruction:

static struct lra_insn_reg *
new_insn_reg (int regno, enum op_type type, enum machine_mode mode,
	      bool subreg_p, bool early_clobber, struct lra_insn_reg *next)
{
  struct lra_insn_reg *ir;

  ir = (struct lra_insn_reg *) pool_alloc (insn_reg_pool);
  ir->type = type;
  ir->biggest_mode = mode;
  if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (lra_reg_info[regno].biggest_mode))
    lra_reg_info[regno].biggest_mode = mode;

This then affects allocation decisions in several places.  Here too I think
we need to guard for nondebug insns, although we still need to record
register references for debug insns.

This of course means that when substituting the allocation into the debug
insns, we have to cope with paradoxical subregs that are wider than the
largest recorded mode.  I don't think that's a new requirement though,
since we needed the same thing for reload.  AFAIK the existing code
should already handle it.

The patch below fixes the wide-int problem for me.  Bootstrapped &
regression-tested on trunk for x86_64-linux-gnu.  OK to install?

Thanks,
Richard


gcc/
	* ira.c (update_equiv_regs): Only call set_paradoxical_subreg
	for non-debug insns.
	* lra.c (new_insn_reg): Take the containing insn as a parameter.
	Only modify lra_reg_info[].biggest_mode if it's non-debug insn.
	(collect_non_operand_hard_regs, add_regs_to_insn_regno_info): Update
	accordingly.
Steven Bosscher - Sept. 7, 2013, 11:22 a.m.
On Sat, Sep 7, 2013 at 11:14 AM, Richard Sandiford wrote:
> The problem seems to be split across IRA and LRA.  In IRA we have:
>
>   FOR_EACH_BB (bb)
>     FOR_BB_INSNS (bb, insn)
>       {
>         if (! INSN_P (insn))
>           continue;
>         for_each_rtx (&insn, set_paradoxical_subreg, (void *)pdx_subregs);
>       }
>
> which sets pdx_subregs[R] if R is used in a paradoxical subreg, followed by:
>
>           /* Don't set reg (if pdx_subregs[regno] == true) equivalent to a mem.  */
>           if (MEM_P (src) && pdx_subregs[regno])
>             {
>               note_stores (set, no_equiv, NULL);
>               continue;
>             }
>
> I think this should only happen when a paradoxical subreg is seen
> in a nondebug insn.

Agreed. Otherwise these debug insns would have influence on the
generated code and that's not supposed to happen.


> The patch below fixes the wide-int problem for me.  Bootstrapped &
> regression-tested on trunk for x86_64-linux-gnu.  OK to install?
>
> Thanks,
> Richard
>
>
> gcc/
>         * ira.c (update_equiv_regs): Only call set_paradoxical_subreg
>         for non-debug insns.
>         * lra.c (new_insn_reg): Take the containing insn as a parameter.
>         Only modify lra_reg_info[].biggest_mode if it's non-debug insn.
>         (collect_non_operand_hard_regs, add_regs_to_insn_regno_info): Update
>         accordingly.

I don't think this falls under "RTL optimizers", but the patch looks OK to me.

Can you please add a test case?

Ciao!
Steven
Richard Sandiford - Sept. 7, 2013, 11:37 a.m.
Steven Bosscher <stevenb.gcc@gmail.com> writes:
> Can you please add a test case?

What kind of test would you suggest?  Do we have a harness for testing
that -O2 and -O2 -g .text output is identical?

I suppose the main testcase will be bootstrap if wide-int ever does get
accepted.

Thanks,
Richard
Steven Bosscher - Sept. 7, 2013, 11:39 a.m.
On Sat, Sep 7, 2013 at 1:37 PM, Richard Sandiford wrote:
> Steven Bosscher <stevenb.gcc@gmail.com> writes:
>> Can you please add a test case?
>
> What kind of test would you suggest?  Do we have a harness for testing
> that -O2 and -O2 -g .text output is identical?


Not .text, but the assembly output or the final RTL dumps. This is
what the guality test framework does.

Ciao!
Steven
Jakub Jelinek - Sept. 7, 2013, 11:42 a.m.
On Sat, Sep 07, 2013 at 12:37:14PM +0100, Richard Sandiford wrote:
> Steven Bosscher <stevenb.gcc@gmail.com> writes:
> > Can you please add a test case?
> 
> What kind of test would you suggest?  Do we have a harness for testing
> that -O2 and -O2 -g .text output is identical?

No, but we have -fcompare-debug, which is even stronger than that (compares
final RTL, so can catch even differences that aren't visible in the .s file,
but still are relevant).  And, the default way of bootstrapping also
compares .text etc. after stripping, between stage2 (built without debug)
and stage3 (with debug).

	Jakub
Richard Sandiford - Sept. 7, 2013, 11:55 a.m.
Jakub Jelinek <jakub@redhat.com> writes:
> On Sat, Sep 07, 2013 at 12:37:14PM +0100, Richard Sandiford wrote:
>> Steven Bosscher <stevenb.gcc@gmail.com> writes:
>> > Can you please add a test case?
>> 
>> What kind of test would you suggest?  Do we have a harness for testing
>> that -O2 and -O2 -g .text output is identical?
>
> No, but we have -fcompare-debug, which is even stronger than that (compares
> final RTL, so can catch even differences that aren't visible in the .s file,
> but still are relevant).  And, the default way of bootstrapping also
> compares .text etc. after stripping, between stage2 (built without debug)
> and stage3 (with debug).

Right, the bootstrap thing is what forced me to write the patch in
the first place.  I probably wasn't clear, sorry, but the original
problem was a bootstrap comparison failure between stage 2 and stage 3
because the tree.o code depended on whether debug info was enabled.

I'll try to use -fcompare-debug though, thanks.

Richard

Patch

Index: gcc/ira.c
===================================================================
--- gcc/ira.c	2013-09-01 12:39:40.739835911 +0100
+++ gcc/ira.c	2013-09-07 09:42:34.993934881 +0100
@@ -2944,11 +2944,8 @@  update_equiv_regs (void)
      prevent access beyond allocated memory for paradoxical memory subreg.  */
   FOR_EACH_BB (bb)
     FOR_BB_INSNS (bb, insn)
-      {
-	if (! INSN_P (insn))
-	  continue;
-	for_each_rtx (&insn, set_paradoxical_subreg, (void *)pdx_subregs);
-      }
+      if (NONDEBUG_INSN_P (insn))
+	for_each_rtx (&insn, set_paradoxical_subreg, (void *) pdx_subregs);
 
   /* Scan the insns and find which registers have equivalences.  Do this
      in a separate scan of the insns because (due to -fcse-follow-jumps)
Index: gcc/lra.c
===================================================================
--- gcc/lra.c	2013-07-17 08:36:09.260013240 +0100
+++ gcc/lra.c	2013-09-07 09:42:35.004934955 +0100
@@ -480,13 +480,13 @@  init_insn_regs (void)
     = create_alloc_pool ("insn regs", sizeof (struct lra_insn_reg), 100);
 }
 
-/* Create LRA insn related info about referenced REGNO with TYPE
-   (in/out/inout), biggest reference mode MODE, flag that it is
+/* Create LRA insn related info about a reference to REGNO in INSN with
+   TYPE (in/out/inout), biggest reference mode MODE, flag that it is
    reference through subreg (SUBREG_P), flag that is early clobbered
    in the insn (EARLY_CLOBBER), and reference to the next insn reg
    info (NEXT).	 */
 static struct lra_insn_reg *
-new_insn_reg (int regno, enum op_type type, enum machine_mode mode,
+new_insn_reg (rtx insn, int regno, enum op_type type, enum machine_mode mode,
 	      bool subreg_p, bool early_clobber, struct lra_insn_reg *next)
 {
   struct lra_insn_reg *ir;
@@ -494,7 +494,8 @@  new_insn_reg (int regno, enum op_type ty
   ir = (struct lra_insn_reg *) pool_alloc (insn_reg_pool);
   ir->type = type;
   ir->biggest_mode = mode;
-  if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (lra_reg_info[regno].biggest_mode))
+  if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (lra_reg_info[regno].biggest_mode)
+      && NONDEBUG_INSN_P (insn))
     lra_reg_info[regno].biggest_mode = mode;
   ir->subreg_p = subreg_p;
   ir->early_clobber = early_clobber;
@@ -976,7 +977,7 @@  collect_non_operand_hard_regs (rtx *x, l
 		     && ! (FIRST_STACK_REG <= regno
 			   && regno <= LAST_STACK_REG));
 #endif
-		list = new_insn_reg (regno, type, mode, subreg_p,
+		list = new_insn_reg (data->insn, regno, type, mode, subreg_p,
 				     early_clobber, list);
 	      }
 	  }
@@ -1575,7 +1576,7 @@  add_regs_to_insn_regno_info (lra_insn_re
       expand_reg_info ();
       if (bitmap_set_bit (&lra_reg_info[regno].insn_bitmap, uid))
 	{
-	  data->regs = new_insn_reg (regno, type, mode, subreg_p,
+	  data->regs = new_insn_reg (data->insn, regno, type, mode, subreg_p,
 				     early_clobber, data->regs);
 	  return;
 	}
@@ -1587,8 +1588,9 @@  add_regs_to_insn_regno_info (lra_insn_re
 		if (curr->subreg_p != subreg_p || curr->biggest_mode != mode)
 		  /* The info can not be integrated into the found
 		     structure.  */
-		  data->regs = new_insn_reg (regno, type, mode, subreg_p,
-					     early_clobber, data->regs);
+		  data->regs = new_insn_reg (data->insn, regno, type, mode,
+					     subreg_p, early_clobber,
+					     data->regs);
 		else
 		  {
 		    if (curr->type != type)