Patchwork RFA: dead_debug_* ICE

login
register
mail settings
Submitter Alexandre Oliva
Date June 27, 2012, 10:06 a.m.
Message ID <or4npx9iie.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/167594/
State New
Headers show

Comments

Alexandre Oliva - June 27, 2012, 10:06 a.m.
On Jun 25, 2012, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Jun 24, 2012, Richard Sandiford <rdsandiford@googlemail.com> wrote:
>> gcc.c-torture/compile/vector-2.c fails on mips64-elf with RTL checking
>> enabled because dead_debug_insert_temp tries to read the REGNO of something
>> that it has already replaced with a debug temporary.

> This sounds like http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53740

> The patch looks reasonable to me, but I don't think I'm entitled to
> approve it.

So, it turned out that the observable problems were a red herring.  The
problem was that we were failing to emit debug temps for regs that were
set *and* used, but that died before their last debug use.  The “else”
that I'd recently introduced was a mistake, for it stopped us from
satisfying the needed debug binding with the correct expression if it
had any nondebug uses.  The need would get past (backwards) the setting
point, reaching some earlier set that happened to be actually dead and
getting us thoroughly confused.  This was obviously not supposed to
happen.

Once I removed the incorrectly-added “else”s, the problem no longer
occurred, and I'm pretty sure it won't.

Now, while investigating the problem, I noticed a number of suspicious
paradoxical SUBREGs that I'm pretty sure were supposed to refer to full
double words rather than extending single words to double.  Indeed, we
had a problem in tracking single-reg sets for multi-reg debug uses: we'd
use the value stored in the lowest-numbered as if it was the whole
multireg value.  This would be a sign extension of the low part given
the right endianness, but it would become utter gibberish for the wrong
one.  There was code in place to avoid this incorrect use if we happened
to be storing the part of the value in a SUBREG, but if we wrote to any
entire REG, we'd happily do the wrong thing.

I have plans on how to deal with multiregs properly, as noted in the
newly-added comments, but I haven't decided it's worth tackling yet.

Richard, is it easy for you to confirm that the patch fixes the mips64-
problem too?  If not, I'll build a cross and hope I can trigger the
problem with it.  (no chance of my building an rtx-checking native on my
poor yeeloong ;-)

This was regstrapped on x86_64- and i686-linux-gnu.  I'm checking it in
as obvious after catching some sleep.

Patch

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/53740
	PR debug/52983
	PR debug/48866
	* dce.c (word_dce_process_block): Check whether inserting debug
	temps are needed even for needed insns.
	(dce_process_block): Likewise.
	* df-problems.c (dead_debug_add): Add comment about multi-regs.
	(dead_debug_insert_temp): Likewise.  Don't subreg when we're
	setting fewer regs than a multi-reg requires.

Index: gcc/dce.c
===================================================================
--- gcc/dce.c.orig	2012-06-27 02:29:32.290377543 -0300
+++ gcc/dce.c	2012-06-27 02:30:56.072721259 -0300
@@ -864,9 +864,12 @@  word_dce_process_block (basic_block bb, 
 	   anything in local_live.  */
 	if (marked_insn_p (insn))
 	  df_word_lr_simulate_uses (insn, local_live);
+
 	/* Insert debug temps for dead REGs used in subsequent debug
-	   insns.  */
-	else if (debug.used && !bitmap_empty_p (debug.used))
+	   insns.  We may have to emit a debug temp even if the insn
+	   was marked, in case the debug use was after the point of
+	   death.  */
+	if (debug.used && !bitmap_empty_p (debug.used))
 	  {
 	    df_ref *def_rec;
 
@@ -963,9 +966,12 @@  dce_process_block (basic_block bb, bool 
 	   anything in local_live.  */
 	if (needed)
 	  df_simulate_uses (insn, local_live);
+
 	/* Insert debug temps for dead REGs used in subsequent debug
-	   insns.  */
-	else if (debug.used && !bitmap_empty_p (debug.used))
+	   insns.  We may have to emit a debug temp even if the insn
+	   was marked, in case the debug use was after the point of
+	   death.  */
+	if (debug.used && !bitmap_empty_p (debug.used))
 	  for (def_rec = DF_INSN_DEFS (insn); *def_rec; def_rec++)
 	    dead_debug_insert_temp (&debug, DF_REF_REGNO (*def_rec), insn,
 				    DEBUG_TEMP_BEFORE_WITH_VALUE);
Index: gcc/df-problems.c
===================================================================
--- gcc/df-problems.c.orig	2012-06-27 02:30:45.000000000 -0300
+++ gcc/df-problems.c	2012-06-27 02:30:56.073721179 -0300
@@ -3179,6 +3179,9 @@  dead_debug_add (struct dead_debug *debug
   if (!debug->used)
     debug->used = BITMAP_ALLOC (NULL);
 
+  /* ??? If we dealt with split multi-registers below, we should set
+     all registers for the used mode in case of hardware
+     registers.  */
   bitmap_set_bit (debug->used, uregno);
 }
 
@@ -3269,6 +3272,15 @@  dead_debug_insert_temp (struct dead_debu
 	  /* Hmm...  Something's fishy, we should be setting REG here.  */
 	  if (REGNO (dest) != REGNO (reg))
 	    breg = NULL;
+	  /* If we're not overwriting all the hardware registers that
+	     setting REG in its mode would, we won't know what to bind
+	     the debug temp to.  ??? We could bind the debug_expr to a
+	     CONCAT or PARALLEL with the split multi-registers, and
+	     replace them as we found the corresponding sets.  */
+	  else if (REGNO (reg) < FIRST_PSEUDO_REGISTER
+		   && (hard_regno_nregs[REGNO (reg)][GET_MODE (reg)]
+		       != hard_regno_nregs[REGNO (reg)][GET_MODE (dest)]))
+	    breg = NULL;
 	  /* Ok, it's the same (hardware) REG, but with a different
 	     mode, so SUBREG it.  */
 	  else