Bug 47858 - [4.5/4.6/4.7 Regression] IPA-SRA decreases quality of debug info
Summary: [4.5/4.6/4.7 Regression] IPA-SRA decreases quality of debug info
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: debug (show other bugs)
Version: 4.6.0
: P2 normal
Target Milestone: 4.5.4
Assignee: Jakub Jelinek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-23 13:36 UTC by Jakub Jelinek
Modified: 2011-11-28 15:16 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-02-23 15:00:08


Attachments
gcc47-pr47858.patch (7.04 KB, patch)
2011-06-02 12:21 UTC, Jakub Jelinek
Details | Diff
gcc47-pr47858.patch (7.90 KB, patch)
2011-06-03 10:46 UTC, Jakub Jelinek
Details | Diff
gcc47-pr47858.patch (11.18 KB, patch)
2011-06-07 15:43 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2011-02-23 13:36:18 UTC
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.
Comment 1 Richard Biener 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).
Comment 2 Alexandre Oliva 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?
Comment 3 Richard Biener 2011-04-28 14:51:48 UTC
GCC 4.5.3 is being released, adjusting target milestone.
Comment 4 Jakub Jelinek 2011-06-02 12:21:51 UTC
Created attachment 24416 [details]
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]);
Comment 5 Jakub Jelinek 2011-06-03 10:46:40 UTC
Created attachment 24422 [details]
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.
Comment 6 Jakub Jelinek 2011-06-07 15:43:52 UTC
Created attachment 24459 [details]
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.
Comment 7 Jakub Jelinek 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
Comment 8 Jakub Jelinek 2011-11-28 15:16:59 UTC
Fixed.