On Jun 25, 2012, Alexandre Oliva <aol...@redhat.com> wrote: > On Jun 24, 2012, Richard Sandiford <rdsandif...@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.
for gcc/ChangeLog from Alexandre Oliva <aol...@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
-- Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist Red Hat Brazil Compiler Engineer