[Bug debug/54953] New: [4.8 Regression] New sra-1.c FAILs on powerpc

[Bug debug/54953] New: [4.8 Regression] New sra-1.c FAILs on powerpc

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54953

             Bug #: 54953
           Summary: [4.8 Regression] New sra-1.c FAILs on powerpc
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: debug
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jakub@gcc.gnu.org
                CC: aoliva@gcc.gnu.org
            Target: powerpc64-linux


When backporting the valtrack.c addition and related changes to 4.7 branch,
I've noticed it regresses some sra-1.c tests that used to pass, in particular
FAIL: gcc.dg/guality/sra-1.c  -O1  line 43 a.i == 4
(ditto for all other > -O1 levels).
The problem is that before fwprop1 we have:
(insn 11 10 12 2 (set (reg:HI 130 [ a$i ])
        (asm_operands:HI ("") ("=r") 0 [
                (reg:HI 131)
            ]
             [
                (asm_input:HI ("0") (null):0)
            ]
             [] sra-1.c:40)) sra-1.c:40 -1
     (nil))
(insn 12 11 13 2 (set (reg:DI 127 [ a$i+-6 ])
        (sign_extend:DI (reg:HI 130 [ a$i ]))) sra-1.c:40 21 {*rs6000.md:447}
     (nil))
(debug_insn 13 12 14 2 (var_location:HI a$i (subreg:HI (reg:DI 127 [ a$i+-6 ])
6)) sra-1.c:40 -1
     (nil))
and reg:HI 130 is really dead, and fwprop1 decides to propagate into the
debug_insn, so after fwprop1 debug_insn 13 contains (reg:HI 130) instead of the
subreg.  Then cprop1 comes and performs fast DCE, which decides to put a debug
temp for reg:HI 130 using DEBUG_TEMP_BEFORE_WITH_VALUE before the asm
instruction, but obviously asm_operands inside of var_location is never going
to expand to anything useful.  dce.c seems to be the only user of
DEBUG_TEMP_BEFORE_WITH_VALUE mode of insertion.  If the insn it is adding it to
is not marked (!marked_insn_p (insn)), then that is certainly the right thing
to do, the insn is going to be removed and we have no other option.  But if it
is
being kept, just debug temp is being added because of debug uses after the
pseudo became dead, then this is counterproductive.

[Bug debug/54953] [4.8 Regression] New sra-1.c FAILs on powerpc

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54953

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-10-17
   Target Milestone|---                         |4.8.0
     Ever Confirmed|0                           |1

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-10-17 16:03:50 UTC ---
I've tried:
--- dce.c.jj1    2012-10-08 21:37:36.000000000 +0200
+++ dce.c    2012-10-17 17:32:19.855891915 +0200
@@ -880,7 +880,9 @@ word_dce_process_block (basic_block bb,

         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);
+                      marked_insn_p (insn)
+                      ? DEBUG_TEMP_AFTER_WITH_REG
+                      : DEBUG_TEMP_BEFORE_WITH_VALUE);
       }

     if (dump_file)
@@ -981,7 +983,9 @@ dce_process_block (basic_block bb, bool
     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);
+                    needed
+                    ? DEBUG_TEMP_AFTER_WITH_REG
+                    : DEBUG_TEMP_BEFORE_WITH_VALUE);
       }

   dead_debug_local_finish (&debug, NULL);
but that isn't enough (well, it seems likely the testcase passes again, but
isn't correct).
The problem is in:
654      /* If there's a single (debug) use of an otherwise unused REG, and
655         the debug use is not part of a larger expression, then it
656         probably doesn't make sense to introduce a new debug temp.  */
657      if (where == DEBUG_TEMP_AFTER_WITH_REG && !uses->next)
658        {
659          rtx next = DF_REF_INSN (uses->use);
660    
661          if (DEBUG_INSN_P (next) && reg == INSN_VAR_LOCATION_LOC (next))
662        {
663          XDELETE (uses);
664          return 0;
665        }
666        }
in dead_debug_insert_temp, which triggers in this case and just does nothing. 
I wonder why that hunk has been added, if the pseudo is live at that point,
surely it doesn't make sense, but if it becomes dead in between the insn and
the single debug use, then inserting a new debug temp is desirable even in that
case.  If it is fine for some reason for df-problems.c uses, perhaps we should
have another DEBUG_TEMP_AFTER_WITH_REG_FORCE where mode or similar that would
do the same thing as DEBUG_TEMP_AFTER_WITH_REG, but not trigger this kind of
?optimization?.

[Bug debug/54953] [4.8 Regression] New sra-1.c FAILs on powerpc

[aoliva at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54953

--- Comment #2 from Alexandre Oliva <aoliva at gcc dot gnu.org> 2012-10-26 07:42:07 UTC ---
Without your poposed change, AFTER_WITH_REG is only used while adding
REG_UNUSED marks to REG defs.  That the REG def is unused means it's going to
be discarded and, when it is, either the debug temp will be reset altogether,
or we'll propagate the expression stored in the REG to uses of the REG (e.g.,
the debug use, if we omit the debug temp, or the debug temp, if we needlessly
add it), or we (should?) emit another debug temp BEFORE_WITH_VALUE and use that
instead of the REG (in the orginal debug use or in the needless debug temp). 
In any of these cases, the debug temp is useless.

All that siad, it's probably not a terribly important optimization, so if the
above wouldn't make sense in a comment before the questionable hunk, we might
as well drop it.  Adding REG_FORCE for this new use is probably fine too.

[Bug debug/54953] [4.8 Regression] New sra-1.c FAILs on powerpc

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54953

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |jakub at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-10-26 08:28:59 UTC ---
Created attachment 28535
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28535
gcc48-pr54953.patch

Ok, I'll test this version then.  Feel free to clarify comments ;)

[Bug debug/54953] [4.8 Regression] New sra-1.c FAILs on powerpc

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54953

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-10-30 08:08:08 UTC ---
Author: jakub
Date: Tue Oct 30 08:08:01 2012
New Revision: 192978

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192978
Log:
    PR debug/54953
    * valtrack.h (DEBUG_TEMP_AFTER_WITH_REG_FORCE): New.
    * valtrack.c (dead_debug_insert_temp): Use emit_debug_insn_after
    even for where == DEBUG_TEMP_AFTER_WITH_REG_FORCE.
    * dce.c (word_dce_process_block, dce_process_block): Pass
    DEBUG_TEMP_AFTER_WITH_REG_FORCE if insn is needed and therefore
    not going to be eliminated.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/dce.c
    trunk/gcc/valtrack.c
    trunk/gcc/valtrack.h

[Bug debug/54953] [4.8 Regression] New sra-1.c FAILs on powerpc

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54953

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-07 11:41:06 UTC ---
Should be fixed now.