[Bug debug/47858] New: [4.5/4.6 Regression] IPA-SRA decreases quality of debug info

[Bug debug/47858] New: [4.5/4.6 Regression] IPA-SRA decreases quality of debug info

[jakub at gcc dot gnu.org]

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

           Summary: [4.5/4.6 Regression] IPA-SRA decreases quality of
                    debug info
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: debug
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jakub@gcc.gnu.org
                CC: aoliva@gcc.gnu.org


struct inode
{
  char i_pad[16];
  unsigned int i_flags;
};
struct dentry
{
  unsigned int d_pad[16];
  struct inode *d_inode;
};

extern int inode_permission (struct inode *, int);
extern int fsnotify_create (struct inode *, struct dentry *);

static inline int
may_create (struct inode *dir, struct dentry *child)
{
  if (child->d_inode)
    return -17;
  if (((dir)->i_flags & 16))
    return -2;
  return inode_permission (dir, 2 | 1);
}

int
foo (struct inode *dir, struct dentry *child)
{
  if (may_create (dir, child) < 0)
    return -1;
  return fsnotify_create (dir, child);
}

int
bar (struct inode *dir, struct dentry *child)
{
  if (may_create (dir, child) < 0)
    return -1;
  return fsnotify_create (dir, child);
}

int
baz (struct inode *dir, struct dentry *child)
{
  if (may_create (dir, child) < 0)
    return -1;
  return fsnotify_create (dir, child);
}

at -Os on x86_64-linux (distilled from Linux kernel) pessimizes debug info
because child parameter in may_create (which isn't emitted inline) doesn't have
location (well, it doesn't have even DW_TAG_formal_parameter in the out of line
DIE, just in the abstract inline DIE).

This could be solvable with my proposed DW_TAG_GNU_call_site stuff and the
(still in drafting DW_OP_GNU_parameter_ref stuff for completely optimized away
parameters - here it isn't completely optimized away, just replaced by
something based on it).

In this testcase it is of course also questionable why IPA-SRC actually
performs this "optimization" when it increases code size and doesn't improve
anything at all.

[Bug debug/47858] [4.5/4.6 Regression] IPA-SRA decreases quality of debug info

[rguenth at gcc dot gnu.org]

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

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.02.23 15:00:08
   Target Milestone|---                         |4.5.3
     Ever Confirmed|0                           |1

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-02-23 15:00:08 UTC ---
The idea is that the load can be CSEd at the call-site.  Something we
can't easily verify and thus should be definitely disabled at -Os.

OTOH I think it's not really worth the trouble in general without
further IPA analysis (and I have IPA-SRA disabled even for -O2 for
openSUSE because of the debuginfo issues).

[Bug debug/47858] [4.5/4.6 Regression] IPA-SRA decreases quality of debug info

[aoliva at gcc dot gnu.org]

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

--- Comment #2 from Alexandre Oliva <aoliva at gcc dot gnu.org> 2011-02-24 13:50:18 UTC ---
I think it would make sense to model this call-site SRA as partial inlining. 
It wouldn't get us perfect debug info, but it would at least give us something
sensible at the inlined entry point, without duplicating all the infrastructure
of partial inlining.  Any reason why it shouldn't be modeled this way?

[Bug debug/47858] [4.5/4.6 Regression] IPA-SRA decreases quality of debug info

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug debug/47858] [4.5/4.6/4.7 Regression] IPA-SRA decreases quality of debug info

[rguenth at gcc dot gnu.org]

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

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.5.3                       |4.5.4

--- Comment #3 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-04-28 14:51:48 UTC ---
GCC 4.5.3 is being released, adjusting target milestone.

[Bug debug/47858] [4.5/4.6/4.7 Regression] IPA-SRA decreases quality of debug info

[jakub at gcc dot gnu.org]

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

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 #4 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-06-02 12:21:51 UTC ---
Created attachment 24416
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24416
gcc47-pr47858.patch

Work in progress patch, so far just for the callee side only and not hooked
into IPA-SRA etc. too much.

main testcase used is:
volatile int vv;

static __attribute__((noinline)) int
f1 (int x, int y, int z)
{
  int a = x * 2;
  int b = y * 2;
  int c = z * 2;
  vv++;
  return x + z;
}

int
u1 (int x)
{
  return f1 (x, 2, 3) + f1 (x, 4, 3) + f1 (x + 6, x, 3) + x;
}

The main changes of this patch which I'd like to discuss are:
1) introduction of new debug stmt kind, DEBUG_SOURCE_BIND (something like
   we talked about on IRC, except due to use of the subcode we can't have flags
   and thus it is a new kind).  It has the source mapping semantics,
   so for GIMPLE POV is considered as a black box statement, which doesn't use
   anything.
   Alternatively, we could have a new tree which would embed a decl in it,
stand
   for black box and not be considered any kind of use.
2) remap_ssa_name creates these s=> debug stmts if possible instead of
resetting
   (replaces with D#N temporary set by s=>)
These two things alone fix the testcase in this comment with -g -O2
-fno-ipa-sra, during expansion it is expanded as ENTRY_VALUE and var-tracking
figures out it is even still available in a register.
For -O2 -g -fipa-sra, for b variable this offers a part of solution, in
particular:
3) add DW_OP_GNU_parameter_ref op, which references the artificial
DW_TAG_formal_parameter and corresponding DEBUG_PARAMETER_REF rtl
We should emit DEBUG y s=> y too when optimizing away the parameter so that the
parameter gets DW_OP_GNU_parameter_ref too (or alternatively expansion could
add those automatically for optimized away parameters directly in the form of
debug insn).

The original testcase from kernel needs __attribute__((noinline)) now on
may_create, otherwise it is inlined and doesn't test the interesting case.
And, for better coverage there should be struct dentry *child2 = child; or
similar at the start of may_create too, to also test vars whose value is
related to the optimized away parameter.  Again, some more work is needed not
to reset the # DEBUG child2 => child_2(D) stmt during eipa_sra, but instead add
# DEBUG D#N s=> child
# DEBUG child2 => D#N
and either again add it for the child parameter too
# DEBUG child s=> child
or handle it during expansion.

That is only the first part of the solution, there needs to be matching
DW_TAG_GNU_call_site_parameter with DW_AT_abstract_origin of the decl which
DW_OP_GNU_parameter_def will reference, with the right value.

This could be either again some special debug stmt kind before the call, or
perhaps could be just the call stmt itself taking DEBUG_EXPR_DECLs as extra
arguments (either normal args, but there is some risk of breaking various
passes), or say setting a flag in GIMPLE_CALL's subcode that it has
DEBUG_EXPR_DECL arguments and if it does, call some slower gimple_call_num_args
variant which would omit from the call DEBUG_EXPR_DECLs at the end.
At RTL level this could be e.g. represented by having DEBUG_EXPRs in
CALL_INSN_FUNCTION_USAGE.  So, we would have say:
  # DEBUG D#7 => 2
  D.2696_2 = f1.isra.0 (x_1(D), 3 [, D#7]);
  # DEBUG D#8 => 4
  D.2697_3 = f1.isra.0 (x_1(D), 3 [, D#8]);
  D.2698_4 = D.2696_2 + D.2697_3;
  D.2699_5 = x_1(D) + 6;
  # DEBUG D#9 => x_1(D)
  D.2700_6 = f1.isra.0 (D.2699_5, 3 [, D#9]);

[Bug debug/47858] [4.5/4.6/4.7 Regression] IPA-SRA decreases quality of debug info

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24416|0                           |1
        is obsolete|                            |

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-06-03 10:46:40 UTC ---
Created attachment 24422
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24422
gcc47-pr47858.patch

Updated patch, just added tree-sra.c bits for ipa-sra to add DEBUG x s=> y
and DEBUG z => D#N stmts as needed, and create VAR_DECL for the omitted
arguments.  With this DW_OP_GNU_parameter_ref is generated both for all places
in #c0 and #c4 testcases where it should.

Thinking more about DW_OP_GNU_parameter_ref vs. typed DWARF stack, I think it
will need as optional parameter the base type die, because unlike
DW_OP_GNU_entry_value where the type is derived from the type of the inner
expression (e.g. regval_type vs. breg/reg and deref_type vs. deref{,_size}) the
DWARF consumer needs to know which type should be used for the parameter.
So, it could be DW_OP_GNU_parameter_ref <uleb128, data4> where the first
argument is either 0 (untyped) or base type DIE offset, and second argument is
DIE offset of the DW_TAG_formal_parameter.
Or, it could have no explicit type and the type would be taken from the caller
DW_TAG_GNU_call_site_parameter's DW_AT_GNU_call_site_value.
Preferences from debug consumer folks?

For the (still unimplemented) caller side representation in GIMPLE and RTL,
I forgot we don't need just list of DEBUG_EXPR_DECLs/DEBUG_EXPRs, we actually
need a list of pairs <PARM_DECL, DEBUG_EXPR_DECL/DEBUG_EXPR> and propagate that
list up to var-tracking.

[Bug debug/47858] [4.5/4.6/4.7 Regression] IPA-SRA decreases quality of debug info

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24422|0                           |1
        is obsolete|                            |

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-06-07 15:43:52 UTC ---
Created attachment 24459
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24459
gcc47-pr47858.patch

Updated patch, which generates both caller and callee side of the debug info,
so far tested just on these two testcases and nothing else.

[Bug debug/47858] [4.5/4.6/4.7 Regression] IPA-SRA decreases quality of debug info

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-06-22 10:42:02 UTC ---
Author: jakub
Date: Wed Jun 22 10:41:58 2011
New Revision: 175288

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=175288
Log:
    PR debug/47858
    * gimple.h (enum gimple_debug_subcode): Add GIMPLE_DEBUG_SOURCE_BIND.
    (gimple_build_debug_source_bind_stat): New prototype.
    (gimple_build_debug_source_bind): Define.
    (gimple_debug_source_bind_p, gimple_debug_source_bind_get_var,
    gimple_debug_source_bind_get_value,
    gimple_debug_source_bind_get_value_ptr,
    gimple_debug_source_bind_set_var,
    gimple_debug_source_bind_set_value): New inlines.
    * gimple.c (gimple_build_debug_source_bind_stat): New function.
    * gimple-pretty-print.c (dump_gimple_debug): Handle
    GIMPLE_DEBUG_SOURCE_BIND.
    * sese.c (rename_uses): Handle gimple_debug_source_bind_p.
    * tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Likewise.
    * tree-parloops.c (eliminate_local_variables,
    separate_decls_in_region): Likewise.
    (separate_decls_in_region_debug): Renamed from
    separate_decls_in_region_debug_bind.  Handle
    gimple_debug_source_bind_p.
    * tree.h (decl_debug_args_lookup, decl_debug_args_insert): New
    prototypes.
    (DECL_HAS_DEBUG_ARGS_P): Define.
    (struct tree_function_decl): Add has_debug_args_flag field.
    * tree.c (debug_args_for_decl): New variable.
    (decl_debug_args_lookup, decl_debug_args_insert): New functions.
    * tree-into-ssa.c (mark_def_sites): Handle uses in debug stmts.
    (rewrite_debug_stmt_uses): New function.
    (rewrite_stmt): Use it to rewrite debug stmt uses.
    * rtl.def (DEBUG_PARAMETER_REF): New.
    * rtl.h (DEBUG_PARAMETER_REF_DECL): Define.
    * cselib.c (rtx_equal_for_cselib_1, cselib_hash_rtx): Handle
    DEBUG_PARAMETER_REF.
    * rtl.c (rtx_equal_p_cb, rtx_equal_p, iterative_hash_rtx): Likewise.
    * print-rtl.c (print_rtx): Likewise.
    * tree-sra.c (sra_ipa_reset_debug_stmts): Prefer replacing of
    SSA_NAMEs with DEBUG_EXPR_DECLs initialized in source bind
    debug stmts in the first bb.
    * tree-inline.c (remap_ssa_name): If remapping default def
    of a PARM_DECL fails, map to a DEBUG_EXPR_DECL set in
    a source bind debug stmt.
    (remap_gimple_stmt): Handle gimple_debug_source_bind_p.
    (maybe_move_debug_stmts_to_successors): Likewise.
    (copy_debug_stmt): Likewise.  Avoid shadowing a variable.
    (tree_function_versioning): If DECL_HAS_DEBUG_ARGS_P, copy
    debug args vector from old_decl to new_decl.
    * ipa-prop.c (ipa_modify_call_arguments): For optimized away
    or modified parameters, add debug bind stmts before call
    setting DEBUG_EXPR_DECL which is remembered in debug args
    vector.
    * cfgexpand.c (expand_call_stmt): Call expand_debug_expr
    on DECL_DEBUG_EXPRs from debug args vector.
    (expand_debug_source_expr): New function.
    (expand_debug_locations): Use it for source bind insns.
    (expand_gimple_basic_block): Handle gimple_debug_source_bind_p.
    * var-tracking.c (prepare_call_arguments): Add debug args
    to call_arguments if any.
    * dwarf2out.c (dwarf_stack_op_name, size_of_loc_descr,
    output_loc_operands, output_loc_operands_raw,
    resolve_addr_in_expr, compare_loc_operands): Handle
    DW_OP_GNU_parameter_ref.
    (get_ref_die_offset, parameter_ref_descriptor): New functions.
    (mem_loc_descriptor): Handle DEBUG_PARAMETER_REF.
    (gen_subprogram_die): Handle parameters identified by
    DEBUG_PARAMETER_REF.

    * dwarf2.h (enum dwarf_location_atom): Add DW_OP_GNU_parameter_ref.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cfgexpand.c
    trunk/gcc/cselib.c
    trunk/gcc/dwarf2out.c
    trunk/gcc/gimple-pretty-print.c
    trunk/gcc/gimple.c
    trunk/gcc/gimple.h
    trunk/gcc/ipa-prop.c
    trunk/gcc/print-rtl.c
    trunk/gcc/rtl.c
    trunk/gcc/rtl.def
    trunk/gcc/rtl.h
    trunk/gcc/sese.c
    trunk/gcc/tree-inline.c
    trunk/gcc/tree-into-ssa.c
    trunk/gcc/tree-parloops.c
    trunk/gcc/tree-sra.c
    trunk/gcc/tree-ssa-dce.c
    trunk/gcc/tree.c
    trunk/gcc/tree.h
    trunk/gcc/var-tracking.c
    trunk/include/ChangeLog
    trunk/include/dwarf2.h

[Bug debug/47858] [4.5/4.6/4.7 Regression] IPA-SRA decreases quality of debug info

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-11-28 15:16:59 UTC ---
Fixed.