[Bug debug/54551] New: DF resets some DEBUG_INSNs unnecessarily

[Bug debug/54551] New: DF resets some DEBUG_INSNs unnecessarily

[jakub at gcc dot gnu.org]

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

             Bug #: 54551
           Summary: DF resets some DEBUG_INSNs unnecessarily
    Classification: Unclassified
           Product: gcc
           Version: 4.7.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: debug
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jakub@gcc.gnu.org
                CC: aoliva@gcc.gnu.org, hubicka@gcc.gnu.org,
                    jan.kratochvil@redhat.com, mark@gcc.gnu.org,
                    rguenth@gcc.gnu.org
        Depends on: 54519


+++ This bug was initially created as a clone of Bug #54519 +++

Several of the PR54519 tests fail.  The problem can be reproduced even without
any partial inlining though, e.g. on:
void bar (void);

int
foo (int x, int y, int z)
{
  if (x != z)
    {
      int a = z + 8;
      bar ();
      bar ();
    }
  return y;
}
at -g -O2.  At *.dfinit we still have:
(insn 4 3 5 2 (set (reg/v:SI 62 [ z ])
        (reg:SI 1 dx [ z ])) vu.c:5 65 {*movsi_internal}
     (nil))
(note 5 4 8 2 NOTE_INSN_FUNCTION_BEG)
(insn 8 5 9 2 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg/v:SI 60 [ x ])
            (reg/v:SI 62 [ z ]))) vu.c:6 7 {*cmpsi_1}
     (nil))
(jump_insn 9 8 10 2 (set (pc)
        (if_then_else (eq (reg:CCZ 17 flags)
                (const_int 0 [0]))
            (label_ref 14)
            (pc))) vu.c:6 595 {*jcc_1}
     (expr_list:REG_BR_PROB (const_int 3784 [0xec8])
        (nil))
 -> 14)
(note 10 9 11 3 [bb 3] NOTE_INSN_BASIC_BLOCK)
(debug_insn 11 10 12 3 (var_location:SI a (plus:SI (reg/v:SI 62 [ z ])
        (const_int 8 [0x8]))) vu.c:8 -1
     (nil))
but during CSE1 when fast DCE is performed, the debug insn for a is reset, as
pseudo 62 isn't live in that basic block.  We have the valtrack.c
infrastructure for this kind of things, but that apparently works only within
basic blocks, while in this case we have a BB (2) where the pseudo dies and a
BB (3) that is dominated by that BB and has a debug insn using that pseudo.
Perhaps in further RTL passes that use DF that is sufficient, but the first
time DF liveness is computed as this testcase or PR54519 shows we drop on the
floor debug insns that could still refer to debug temporaries if we initialized
them in the bbs where they die and that dominate the debug uses.  For this
particular testcase it could still live in %edx on the first call bar insn,
then in DW_OP_GNU_entry_value (%edx).

Alex, what do you think about this?

[Bug debug/54551] DF resets some DEBUG_INSNs unnecessarily

[aoliva at gcc dot gnu.org]

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

--- Comment #1 from Alexandre Oliva <aoliva at gcc dot gnu.org> 2012-09-11 21:20:11 UTC ---
I guess we have to somehow local all death points of the pseudo in paths
towards the debug use, and insert debug insns binding the same debug temp to
the pseudo before all of the death points, then replace the debug use with a
use of the debug temp.  I'm not sure how well this fits in the general
structure of the DF machinery.  Presumably we just need to look up a table of
(lists of?) debug temps as we reach death points.

[Bug debug/54551] DF resets some DEBUG_INSNs unnecessarily

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-09-12 05:54:29 UTC ---
If there is a death point of the pseudo that dominates bbs with uses in some
debug insns, then I think best is to insert the debug temporary immediately
before the death point.  If the death point of the pseudo doesn't dominate the
bb with debug uses, or if there are multiple death point in different branches,
but if the setter of the pseudo dominates the bb with debug uses or if there is
some bb where the pseudo is live, isn't changed afterwards and that spot
dominates the debug uses, then the best spot to insert the debug temporary is
probably before the conditional jump/whatever other control changing insn at
the end of that bb.  E.g. for:
+---+      |
|set|     / \
+---+  +---++---+
  |    |set||set|
 / \   +---++---+
 | |      | |
 \ /      \ /
 (1)       |
+-----+   (2)
|death|   / \
+-----+  |   \
  |   +-----+ \
 / \  |death| +-----+
 |  | +-----+ |death|
 | / \      | +-----+
 | | |      \ /
 | \ /       |
 |  |        +------+
 | +------+  |dbguse|
 | |dbguse|  +------+
 | +------+

I think we want to insert the debug temp at (1) resp. (2).  If there is no such
spot, I think we have to give up, trying to build (if_then_else (condition)
D#1234 D#2345) would bloat the debug info too much. Still handling even the
dominating cases would be better than what we have right now.

Perhaps we could handle single setters first if DF has computed that already.
Perhaps this handling could be keyed off some new DF flag which would only be
set in the first cse pass.

[Bug debug/54551] DF resets some DEBUG_INSNs unnecessarily

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-09-12 07:38:39 UTC ---
Inefficient way to handle at least the single setter case would be at the start
of the bb with non-empty debug uses bitmap (i.e. what is about to be reset)
walk the immediate dominators up, looking for where DF_REG_DEF_COUNT (regno) ==
1
pseudos (and perhaps with additional check that the single DF_REG_DEF_CHAIN
(regno) insn's bb dominates the current bb) start to be live (and stop the walk
at the setter bb).  If it isn't live in lr at the end of some dominator bb but
is live in lr at the start, then perhaps walk DF_REG_REF_CHAIN looking for refs
inside of that bb, or walk the bb backwards looking for last non-debug use,
insert before that), if it isn't live in lr at the start of a bb but is live in
lr at the end of its immediate dominator, then insert debug temp before the end
of that bb, and if it isn't live at the end of the setter bb, look also for the
last non-debug use before the set.

[Bug debug/54551] DF resets some DEBUG_INSNs unnecessarily

[aoliva at gcc dot gnu.org]

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

Alexandre Oliva <aoliva at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-09-23
         AssignedTo|unassigned at gcc dot       |aoliva at gcc dot gnu.org
                   |gnu.org                     |
     Ever Confirmed|0                           |1

--- Comment #4 from Alexandre Oliva <aoliva at gcc dot gnu.org> 2012-09-23 11:36:13 UTC ---
Mine.  Patch posted at http://gcc.gnu.org/ml/gcc-patches/2012-09/msg01606.html

[Bug debug/54551] DF resets some DEBUG_INSNs unnecessarily

[aoliva at gcc dot gnu.org]

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

--- Comment #5 from Alexandre Oliva <aoliva at gcc dot gnu.org> 2012-10-02 20:06:13 UTC ---
Author: aoliva
Date: Tue Oct  2 20:06:08 2012
New Revision: 192001

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192001
Log:
gcc/ChangeLog:
PR debug/54551
* Makefile.in (VALTRACK_H): Add hash-table.h.
* valtrack.h: Include hash-table.h.
(struct dead_debug_global_entry): New.
(struct dead_debug_hash_descr): New.
(struct dead_debug_global): New.
(struct dead_debug): Rename to...
(struct dead_debug_local): ... this.  Adjust all uses.
(dead_debug_global_init, dead_debug_global_finish): New.
(dead_debug_init): Rename to...
(dead_debug_local_init): ... this.  Adjust all callers.
(dead_debug_finish): Rename to...
(dead_debug_local_finish): ... this.  Adjust all callers.
* valtrack.c (dead_debug_global_init): New.
(dead_debug_init): Rename to...
(dead_debug_local_init): ... this.  Take global parameter.
Save it and initialize used bitmap from it.
(dead_debug_global_find, dead_debug_global_insert): New.
(dead_debug_global_replace_temp): New.
(dead_debug_promote_uses): New.
(dead_debug_finish): Rename to...
(dead_debug_local_finish): ... this.  Promote remaining uses.
(dead_debug_global_finish): New.
(dead_debug_add): Try to replace global temps first.
(dead_debug_insert_temp): Support global replacements.
* dce.c (word_dce_process_block, dce_process_block): Add
global_debug parameter.  Pass it on.
(fast_dce): Initialize, pass on and finalize global_debug.
* df-problems.c (df_set_unused_notes_for_mw): Adjusted.
(df_create_unused_notes, df_note_bb_compute): Likewise.
(df_note_compute): Justify local-only dead debug analysis.
gcc/testsuite/ChangeLog:
PR debug/54551
* gcc.dg/guality/pr54551.c: New.

Added:
    trunk/gcc/testsuite/gcc.dg/guality/pr54551.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/Makefile.in
    trunk/gcc/dce.c
    trunk/gcc/df-problems.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/valtrack.c
    trunk/gcc/valtrack.h

[Bug debug/54551] DF resets some DEBUG_INSNs unnecessarily

[aoliva at gcc dot gnu.org]

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

--- Comment #6 from Alexandre Oliva <aoliva at gcc dot gnu.org> 2012-10-02 20:16:33 UTC ---
Fixed in the trunk.  Backporting to 4.7...

[Bug debug/54551] DF resets some DEBUG_INSNs unnecessarily

[aoliva at gcc dot gnu.org]

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

--- Comment #7 from Alexandre Oliva <aoliva at gcc dot gnu.org> 2012-10-29 19:27:39 UTC ---
Author: aoliva
Date: Mon Oct 29 19:27:31 2012
New Revision: 192959

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192959
Log:
PR debug/54551
PR debug/54693
* valtrack.c (dead_debug_global_find): Accept NULL dtemp.
(dead_debug_global_insert): Return new entry.
(dead_debug_global_replace_temp): Return early if REG is no
longer in place, or if dtemp was already substituted.
(dead_debug_promote_uses): Insert for all defs and replace all
debug uses at once.
(dead_debug_local_finish): Release used after promotion.
(dead_debug_insert_temp): Stop if dtemp is NULL.

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

[Bug debug/54551] DF resets some DEBUG_INSNs unnecessarily

[aoliva at gcc dot gnu.org]

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

--- Comment #8 from Alexandre Oliva <aoliva at gcc dot gnu.org> 2012-10-30 23:47:43 UTC ---
Author: aoliva
Date: Tue Oct 30 23:47:35 2012
New Revision: 193003

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193003
Log:
PR debug/54551
PR debug/54693
* valtrack.c (dead_debug_promote_uses): Assert-check that
global used bit was clear and initialize entry
unconditionally.

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