| Message ID | 20121003124804.GR1787@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 3 Oct 2012, Jakub Jelinek wrote: > On Tue, Sep 11, 2012 at 03:59:56PM +0200, Jakub Jelinek wrote: > > As discussed in the PR, right now we do a very bad job for debug info > > of partially inlined functions (both when they are kept only partially > > inlined, or when partial inlining is performed, but doesn't seem to be > > useful and foo.part.N is inlined back, either into the original function, or > > into a function into which foo has been inlined first). > > > > This patch improves that by doing something similar to what ipa-prop.c does, > > in particular for arguments that aren't actually passed to foo.part.N > > we add debug args and corresponding debug bind and debug source bind stmts > > to provide better debug info (if foo.part.N isn't inlined, then > > DW_OP_GNU_parameter_ref is going to be used together with corresponding call > > site arguments). > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, some of the tests > > still fail with some option combinations, am going to file a DF VTA PR for > > that momentarily. Ok for trunk? > > Now that Alex fixed the DF VTA issue, most of the non-LTO FAILs are gone, > the remaining one on x86_64 looks like a GDB bug, and there is one -Os > failure on pr54519-3.c which could be either alias oracle or var-tracking > bug/missing feature - basically there is a non-addressable parameter > in stack slot, and vt_canon_true_dep -> canon_true_dependence thinks an > argument push insn might alias with it, because it doesn't have a MEM_EXPR > and ao_ref_from_mem fails. > > Posting an updated patch, because last night I found that even when the > patch should have fixed > static inline void foo (int x, int y) { asm volatile ("nop"); } > static inline void bar (int z) { foo (z, 0); foo (z, 1); } > int main () > { > bar (0); > bar (1); > return 0; > } > it didn't, there was a confusion when DECL_ORIGIN should be used and when > not. This version of the patch fixes that, on this testcase (adjusted > added as pr54519-6.c) p x, p y and up; p z now work well. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2012-10-03 Jakub Jelinek <jakub@redhat.com> > > PR debug/54519 > * ipa-split.c (split_function): Add debug args and > debug source and normal stmts for args_to_skip which are > gimple regs. > * tree-inline.c (copy_debug_stmt): When inlining, adjust > source debug bind stmts to debug binds of corresponding > DEBUG_EXPR_DECL. > > * gcc.dg/guality/pr54519-1.c: New test. > * gcc.dg/guality/pr54519-2.c: New test. > * gcc.dg/guality/pr54519-3.c: New test. > * gcc.dg/guality/pr54519-4.c: New test. > * gcc.dg/guality/pr54519-5.c: New test. > * gcc.dg/guality/pr54519-6.c: New test. > > --- gcc/ipa-split.c.jj 2012-09-25 14:26:52.612821323 +0200 > +++ gcc/ipa-split.c 2012-10-02 17:41:31.030357922 +0200 > @@ -1059,6 +1059,7 @@ split_function (struct split_point *spli > gimple last_stmt = NULL; > unsigned int i; > tree arg, ddef; > + VEC(tree, gc) **debug_args = NULL; > > if (dump_file) > { > @@ -1232,6 +1233,65 @@ split_function (struct split_point *spli > gimple_set_block (call, DECL_INITIAL (current_function_decl)); > VEC_free (tree, heap, args_to_pass); > The following could use a comment on what you are doing ... > + if (args_to_skip) > + for (parm = DECL_ARGUMENTS (current_function_decl), num = 0; > + parm; parm = DECL_CHAIN (parm), num++) > + if (bitmap_bit_p (args_to_skip, num) > + && is_gimple_reg (parm)) > + { > + tree ddecl; > + gimple def_temp; > + > + arg = get_or_create_ssa_default_def (cfun, parm); > + if (!MAY_HAVE_DEBUG_STMTS) > + continue; > + if (debug_args == NULL) > + debug_args = decl_debug_args_insert (node->symbol.decl); > + ddecl = make_node (DEBUG_EXPR_DECL); > + DECL_ARTIFICIAL (ddecl) = 1; > + TREE_TYPE (ddecl) = TREE_TYPE (parm); > + DECL_MODE (ddecl) = DECL_MODE (parm); > + VEC_safe_push (tree, gc, *debug_args, DECL_ORIGIN (parm)); > + VEC_safe_push (tree, gc, *debug_args, ddecl); > + def_temp = gimple_build_debug_bind (ddecl, unshare_expr (arg), > + call); > + gsi_insert_after (&gsi, def_temp, GSI_NEW_STMT); > + } > + if (debug_args != NULL) > + { > + unsigned int i; > + tree var, vexpr; > + gimple_stmt_iterator cgsi; > + gimple def_temp; > + > + push_cfun (DECL_STRUCT_FUNCTION (node->symbol.decl)); What do you need the push/pop_cfun for? I see ENTRY_BLOCK_PTR (easily fixable), but else? > + var = BLOCK_VARS (DECL_INITIAL (node->symbol.decl)); > + i = VEC_length (tree, *debug_args); > + cgsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR)); > + do > + { > + i -= 2; > + while (var != NULL_TREE > + && DECL_ABSTRACT_ORIGIN (var) > + != VEC_index (tree, *debug_args, i)) > + var = TREE_CHAIN (var); > + if (var == NULL_TREE) > + break; > + vexpr = make_node (DEBUG_EXPR_DECL); > + parm = VEC_index (tree, *debug_args, i); > + DECL_ARTIFICIAL (vexpr) = 1; > + TREE_TYPE (vexpr) = TREE_TYPE (parm); > + DECL_MODE (vexpr) = DECL_MODE (parm); > + def_temp = gimple_build_debug_source_bind (vexpr, parm, > + NULL); > + gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT); > + def_temp = gimple_build_debug_bind (var, vexpr, NULL); > + gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT); > + } > + while (i); > + pop_cfun (); > + } > + > /* We avoid address being taken on any variable used by split part, > so return slot optimization is always possible. Moreover this is > required to make DECL_BY_REFERENCE work. */ > --- gcc/tree-inline.c.jj 2012-09-25 14:26:52.576821521 +0200 > +++ gcc/tree-inline.c 2012-10-02 17:43:13.395786581 +0200 > @@ -2374,6 +2374,31 @@ copy_debug_stmt (gimple stmt, copy_body_ > gimple_debug_source_bind_set_var (stmt, t); > walk_tree (gimple_debug_source_bind_get_value_ptr (stmt), > remap_gimple_op_r, &wi, NULL); > + /* When inlining and source bind refers to one of the optimized > + away parameters, change the source bind into normal debug bind > + referring to the corresponding DEBUG_EXPR_DECL that should have > + been bound before the call stmt. */ > + t = gimple_debug_source_bind_get_value (stmt); > + if (t != NULL_TREE > + && TREE_CODE (t) == PARM_DECL > + && id->gimple_call) > + { > + VEC(tree, gc) **debug_args = decl_debug_args_lookup (id->src_fn); > + unsigned int i; > + if (debug_args != NULL) > + { > + for (i = 0; i < VEC_length (tree, *debug_args); i += 2) > + if (VEC_index (tree, *debug_args, i) == DECL_ORIGIN (t) > + && TREE_CODE (VEC_index (tree, *debug_args, i + 1)) > + == DEBUG_EXPR_DECL) > + { > + t = VEC_index (tree, *debug_args, i + 1); > + gimple_debug_source_bind_set_value (stmt, t); > + stmt->gsbase.subcode = GIMPLE_DEBUG_BIND; That looks fishy ... shouldn't it be at least the other way around? stmt->gsbase.subcode = GIMPLE_DEBUG_BIND; gimple_debug_bind_set_value (stmt, t); ? Otherwise this looks ok. Thanks, Richard. > + break; > + } > + } > + } > } > > processing_debug_stmt = 0; > --- gcc/testsuite/gcc.dg/guality/pr54519-2.c.jj 2012-10-02 16:27:24.862658030 +0200 > +++ gcc/testsuite/gcc.dg/guality/pr54519-2.c 2012-10-02 16:27:24.862658030 +0200 > @@ -0,0 +1,45 @@ > +/* PR debug/54519 */ > +/* { dg-do run } */ > +/* { dg-options "-g" } */ > + > +__attribute__((noinline, noclone)) void > +fn1 (int x) > +{ > + __asm volatile ("" : "+r" (x) : : "memory"); > +} > + > +static int > +fn2 (int x, int y) > +{ > + if (y) > + { > + fn1 (x); /* { dg-final { gdb-test 17 "x" "6" } } */ > + fn1 (x); /* { dg-final { gdb-test 17 "y" "25" } } */ > + fn1 (x); > + fn1 (x); > + y = -2 + x; > + y = y * y * y + y; > + fn1 (x + y); /* { dg-final { gdb-test 22 "y" "68" } } */ > + } > + return x; > +} > + > +__attribute__((noinline, noclone)) int > +fn3 (int x, int y) > +{ > + return fn2 (x, y) + y; > +} > + > +__attribute__((noinline, noclone)) int > +fn4 (int x, int y) > +{ > + return fn2 (x, y) + y; > +} > + > +int > +main () > +{ > + fn3 (6, 25); > + fn4 (4, 117); > + return 0; > +} > --- gcc/testsuite/gcc.dg/guality/pr54519-1.c.jj 2012-10-02 16:27:24.862658030 +0200 > +++ gcc/testsuite/gcc.dg/guality/pr54519-1.c 2012-10-02 16:27:24.862658030 +0200 > @@ -0,0 +1,48 @@ > +/* PR debug/54519 */ > +/* { dg-do run } */ > +/* { dg-options "-g" } */ > + > +__attribute__((noinline, noclone)) void > +fn1 (int x) > +{ > + __asm volatile ("" : "+r" (x) : : "memory"); > +} > + > +static int > +fn2 (int x, int y, int z) > +{ > + int a = 8; > + if (x != z) > + { > + fn1 (x); > + fn1 (x); /* { dg-final { gdb-test 20 "x" "36" } } */ > + if (x == 36) /* { dg-final { gdb-test 20 "y" "25" } } */ > + fn1 (x); /* { dg-final { gdb-test 20 "z" "6" } } */ > + fn1 (x); /* { dg-final { gdb-test 23 "x" "98" } } */ > + if (x == 98) /* { dg-final { gdb-test 23 "y" "117" } } */ > + fn1 (x); /* { dg-final { gdb-test 23 "z" "8" } } */ > + fn1 (x); > + fn1 (x + a); > + } > + return y; > +} > + > +__attribute__((noinline, noclone)) int > +fn3 (int x, int y) > +{ > + return fn2 (x, y, 6); > +} > + > +__attribute__((noinline, noclone)) int > +fn4 (int x, int y) > +{ > + return fn2 (x, y, 8); > +} > + > +int > +main () > +{ > + fn3 (36, 25); > + fn4 (98, 117); > + return 0; > +} > --- gcc/testsuite/gcc.dg/guality/pr54519-3.c.jj 2012-10-02 16:27:24.863658017 +0200 > +++ gcc/testsuite/gcc.dg/guality/pr54519-3.c 2012-10-02 16:27:24.863658017 +0200 > @@ -0,0 +1,42 @@ > +/* PR debug/54519 */ > +/* { dg-do run } */ > +/* { dg-options "-g" } */ > + > +__attribute__((noinline, noclone)) void > +fn1 (int x) > +{ > + __asm volatile ("" : "+r" (x) : : "memory"); > +} > + > +static int > +fn2 (int x, int y, int z) > +{ > + int a = 8; > + if (x != z) > + { > + fn1 (x); > + fn1 (x); /* { dg-final { gdb-test 20 "x" "36" } } */ > + if (x == 36) /* { dg-final { gdb-test 20 "y" "25" } } */ > + fn1 (x); /* { dg-final { gdb-test 20 "z" "6" } } */ > + fn1 (x); /* { dg-final { gdb-test 23 "x" "98" } } */ > + if (x == 98) /* { dg-final { gdb-test 23 "y" "117" } } */ > + fn1 (x); /* { dg-final { gdb-test 23 "z" "8" } } */ > + fn1 (x); > + fn1 (x + a); > + } > + return y; > +} > + > +int (*p) (int, int, int) = fn2; > + > +int > +main () > +{ > + __asm volatile ("" : : : "memory"); > + int (*q) (int, int, int) = p; > + __asm volatile ("" : : : "memory"); > + q (36, 25, 6); > + q (98, 117, 8); > + q (0, 0, 0); > + return 0; > +} > --- gcc/testsuite/gcc.dg/guality/pr54519-4.c.jj 2012-10-02 16:27:24.863658017 +0200 > +++ gcc/testsuite/gcc.dg/guality/pr54519-4.c 2012-10-02 16:27:24.863658017 +0200 > @@ -0,0 +1,39 @@ > +/* PR debug/54519 */ > +/* { dg-do run } */ > +/* { dg-options "-g" } */ > + > +__attribute__((noinline, noclone)) void > +fn1 (int x) > +{ > + __asm volatile ("" : "+r" (x) : : "memory"); > +} > + > +static int > +fn2 (int x, int y) > +{ > + if (y) > + { > + fn1 (x); /* { dg-final { gdb-test 17 "x" "6" } } */ > + fn1 (x); /* { dg-final { gdb-test 17 "y" "25" } } */ > + fn1 (x); > + fn1 (x); > + y = -2 + x; > + y = y * y * y + y; > + fn1 (x + y); /* { dg-final { gdb-test 22 "y" "68" } } */ > + } > + return x; > +} > + > +int (*p) (int, int) = fn2; > + > +int > +main () > +{ > + __asm volatile ("" : : : "memory"); > + int (*q) (int, int) = p; > + __asm volatile ("" : : : "memory"); > + q (6, 25); > + q (4, 117); > + q (0, 0); > + return 0; > +} > --- gcc/testsuite/gcc.dg/guality/pr54519-5.c.jj 2012-10-02 16:27:24.863658017 +0200 > +++ gcc/testsuite/gcc.dg/guality/pr54519-5.c 2012-10-02 16:27:24.863658017 +0200 > @@ -0,0 +1,45 @@ > +/* PR debug/54519 */ > +/* { dg-do run } */ > +/* { dg-options "-g" } */ > + > +__attribute__((noinline, noclone)) void > +fn1 (int x) > +{ > + __asm volatile ("" : "+r" (x) : : "memory"); > +} > + > +static int > +fn2 (int x, int y) > +{ > + if (y) > + { > + fn1 (x); /* { dg-final { gdb-test 17 "x" "6" } } */ > + fn1 (x); /* { dg-final { gdb-test 17 "y" "25" } } */ > + fn1 (x); > + fn1 (x); > + y = -2 + x; > + y = y * y * y + y; > + fn1 (x + y); /* { dg-final { gdb-test 22 "y" "68" } } */ > + } > + return x; > +} > + > +__attribute__((noinline, noclone)) int > +fn3 (int x, int y) > +{ > + return fn2 (x, y); > +} > + > +__attribute__((noinline, noclone)) int > +fn4 (int x, int y) > +{ > + return fn2 (x, y); > +} > + > +int > +main () > +{ > + fn3 (6, 25); > + fn4 (4, 117); > + return 0; > +} > --- gcc/testsuite/gcc.dg/guality/pr54519-6.c.jj 2012-10-02 18:03:45.137732997 +0200 > +++ gcc/testsuite/gcc.dg/guality/pr54519-6.c 2012-10-02 18:03:19.000000000 +0200 > @@ -0,0 +1,27 @@ > +/* PR debug/54519 */ > +/* { dg-do run } */ > +/* { dg-options "-g" } */ > + > +#include "../nop.h" > + > +static inline void > +f1 (int x, int y) > +{ > + asm volatile (NOP); /* { dg-final { gdb-test 11 "x" "2" } } */ > + asm volatile (NOP); /* { dg-final { gdb-test 11 "y" "0" } } */ > +} > + > +static inline void > +f2 (int z) > +{ > + f1 (z, 0); > + f1 (z, 1); > +} > + > +int > +main () > +{ > + f2 (2); > + f2 (3); > + return 0; > +} > > > Jakub > >
On Fri, Oct 05, 2012 at 02:20:13PM +0200, Richard Guenther wrote: > The following could use a comment on what you are doing ... Will add something. > > + if (args_to_skip) > > + for (parm = DECL_ARGUMENTS (current_function_decl), num = 0; > > + parm; parm = DECL_CHAIN (parm), num++) > > + if (bitmap_bit_p (args_to_skip, num) > > + && is_gimple_reg (parm)) > > + { > > + tree ddecl; > > + gimple def_temp; > > + > > + arg = get_or_create_ssa_default_def (cfun, parm); > > + if (!MAY_HAVE_DEBUG_STMTS) > > + continue; > > + if (debug_args == NULL) > > + debug_args = decl_debug_args_insert (node->symbol.decl); > > + ddecl = make_node (DEBUG_EXPR_DECL); > > + DECL_ARTIFICIAL (ddecl) = 1; > > + TREE_TYPE (ddecl) = TREE_TYPE (parm); > > + DECL_MODE (ddecl) = DECL_MODE (parm); > > + VEC_safe_push (tree, gc, *debug_args, DECL_ORIGIN (parm)); > > + VEC_safe_push (tree, gc, *debug_args, ddecl); > > + def_temp = gimple_build_debug_bind (ddecl, unshare_expr (arg), > > + call); > > + gsi_insert_after (&gsi, def_temp, GSI_NEW_STMT); > > + } > > + if (debug_args != NULL) > > + { > > + unsigned int i; > > + tree var, vexpr; > > + gimple_stmt_iterator cgsi; > > + gimple def_temp; > > + > > + push_cfun (DECL_STRUCT_FUNCTION (node->symbol.decl)); > > What do you need the push/pop_cfun for? I see ENTRY_BLOCK_PTR > (easily fixable), but else? I believe that gsi_insert_before in another function isn't going to work well. E.g. update_modified_stmt starts with if (!ssa_operands_active (cfun)) return; Or is it ok to use gsi_insert_before_without_update and expect that when compilation resumes for the modified callee, it will update all modified stmts? There is not much that needs to be updated on the stmts, source bind has not setters nor uses of any SSA_NAMEs or virtual defs/sets, the normal bind has a DEBUG_EXPR_DECL on the RHS and so is not a use or def of anything as well. If push_cfun/pop_cfun is not needed, guess the two loops could be merged, the reason for two of them was just that I didn't want to push_cfun/pop_cfun for each optimized away parameter. > > > + var = BLOCK_VARS (DECL_INITIAL (node->symbol.decl)); > > + i = VEC_length (tree, *debug_args); > > + cgsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR)); > > + do > > + { > > + i -= 2; > > + while (var != NULL_TREE > > + && DECL_ABSTRACT_ORIGIN (var) > > + != VEC_index (tree, *debug_args, i)) > > + var = TREE_CHAIN (var); > > + if (var == NULL_TREE) > > + break; > > + vexpr = make_node (DEBUG_EXPR_DECL); > > + parm = VEC_index (tree, *debug_args, i); > > + DECL_ARTIFICIAL (vexpr) = 1; > > + TREE_TYPE (vexpr) = TREE_TYPE (parm); > > + DECL_MODE (vexpr) = DECL_MODE (parm); > > + def_temp = gimple_build_debug_source_bind (vexpr, parm, > > + NULL); > > + gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT); > > + def_temp = gimple_build_debug_bind (var, vexpr, NULL); > > + gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT); > > + } > > + while (i); > > + pop_cfun (); > > + } > > + > > + t = VEC_index (tree, *debug_args, i + 1); > > + gimple_debug_source_bind_set_value (stmt, t); > > + stmt->gsbase.subcode = GIMPLE_DEBUG_BIND; > > That looks fishy ... shouldn't it be at least the other way around? > > stmt->gsbase.subcode = GIMPLE_DEBUG_BIND; > gimple_debug_bind_set_value (stmt, t); It surely can. I was considering whether to create a new wrapper in gimple.h for the subcode change of a debug stmt, but if there are no further places needing this (don't see them right now), that might be overkill. > Otherwise this looks ok. Thanks. Jakub
On Fri, Oct 05, 2012 at 02:49:07PM +0200, Jakub Jelinek wrote: > I believe that gsi_insert_before in another function > isn't going to work well. > E.g. update_modified_stmt starts with > if (!ssa_operands_active (cfun)) > return; > > Or is it ok to use gsi_insert_before_without_update and expect that > when compilation resumes for the modified callee, it will > update all modified stmts? There is not much that needs > to be updated on the stmts, source bind has not setters nor uses > of any SSA_NAMEs or virtual defs/sets, the normal bind has a DEBUG_EXPR_DECL > on the RHS and so is not a use or def of anything as well. To reply to my own post, without_update doesn't work, /usr/src/gcc/gcc/testsuite/gcc.dg/guality/pr54519-1.c: In function 'fn2.part.0': /usr/src/gcc/gcc/testsuite/gcc.dg/guality/pr54519-1.c:12:1: error: stmt (0x7f2d69c3c9d8) marked modified after optimization pass: # DEBUG D#3 s=> z /usr/src/gcc/gcc/testsuite/gcc.dg/guality/pr54519-1.c:12:1: internal # compiler error: verify_ssa failed then kicks in. def_temp = gimple_build_debug_source_bind (vexpr, parm, NULL); /* DEBUG D#N s=> parm stmt doesn't have any SSA defs or uses, no need to update it. */ gimple_set_modified (def_temp, false); gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT); def_temp = gimple_build_debug_bind (var, vexpr, NULL); /* DEBUG decl => D#N' stmt doesn't have any SSA defs or uses, no need to update it. */ gimple_set_modified (def_temp, false); gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT); seems to work, but not sure if that isn't too hackish to avoid one pair of push_cfun/pop_cfun. > If push_cfun/pop_cfun is not needed, guess the two loops could be merged, > the reason for two of them was just that I didn't want to push_cfun/pop_cfun > for each optimized away parameter. I was wrong apparently, the two loops are still needed, as the second loop needs to loop through the parameters in reverse order. Jakub
On Fri, Oct 5, 2012 at 2:49 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Oct 05, 2012 at 02:20:13PM +0200, Richard Guenther wrote: >> The following could use a comment on what you are doing ... > > Will add something. > >> > + if (args_to_skip) >> > + for (parm = DECL_ARGUMENTS (current_function_decl), num = 0; >> > + parm; parm = DECL_CHAIN (parm), num++) >> > + if (bitmap_bit_p (args_to_skip, num) >> > + && is_gimple_reg (parm)) >> > + { >> > + tree ddecl; >> > + gimple def_temp; >> > + >> > + arg = get_or_create_ssa_default_def (cfun, parm); >> > + if (!MAY_HAVE_DEBUG_STMTS) >> > + continue; >> > + if (debug_args == NULL) >> > + debug_args = decl_debug_args_insert (node->symbol.decl); >> > + ddecl = make_node (DEBUG_EXPR_DECL); >> > + DECL_ARTIFICIAL (ddecl) = 1; >> > + TREE_TYPE (ddecl) = TREE_TYPE (parm); >> > + DECL_MODE (ddecl) = DECL_MODE (parm); >> > + VEC_safe_push (tree, gc, *debug_args, DECL_ORIGIN (parm)); >> > + VEC_safe_push (tree, gc, *debug_args, ddecl); >> > + def_temp = gimple_build_debug_bind (ddecl, unshare_expr (arg), >> > + call); >> > + gsi_insert_after (&gsi, def_temp, GSI_NEW_STMT); >> > + } >> > + if (debug_args != NULL) >> > + { >> > + unsigned int i; >> > + tree var, vexpr; >> > + gimple_stmt_iterator cgsi; >> > + gimple def_temp; >> > + >> > + push_cfun (DECL_STRUCT_FUNCTION (node->symbol.decl)); >> >> What do you need the push/pop_cfun for? I see ENTRY_BLOCK_PTR >> (easily fixable), but else? > > I believe that gsi_insert_before in another function > isn't going to work well. > E.g. update_modified_stmt starts with > if (!ssa_operands_active (cfun)) > return; Hmm, indeed. > Or is it ok to use gsi_insert_before_without_update and expect that > when compilation resumes for the modified callee, it will > update all modified stmts? There is not much that needs > to be updated on the stmts, source bind has not setters nor uses > of any SSA_NAMEs or virtual defs/sets, the normal bind has a DEBUG_EXPR_DECL > on the RHS and so is not a use or def of anything as well. I don't think we want to rely on that ... so just keep the push/pop_cfun. Thanks, Richard. > If push_cfun/pop_cfun is not needed, guess the two loops could be merged, > the reason for two of them was just that I didn't want to push_cfun/pop_cfun > for each optimized away parameter. >> >> > + var = BLOCK_VARS (DECL_INITIAL (node->symbol.decl)); >> > + i = VEC_length (tree, *debug_args); >> > + cgsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR)); >> > + do >> > + { >> > + i -= 2; >> > + while (var != NULL_TREE >> > + && DECL_ABSTRACT_ORIGIN (var) >> > + != VEC_index (tree, *debug_args, i)) >> > + var = TREE_CHAIN (var); >> > + if (var == NULL_TREE) >> > + break; >> > + vexpr = make_node (DEBUG_EXPR_DECL); >> > + parm = VEC_index (tree, *debug_args, i); >> > + DECL_ARTIFICIAL (vexpr) = 1; >> > + TREE_TYPE (vexpr) = TREE_TYPE (parm); >> > + DECL_MODE (vexpr) = DECL_MODE (parm); >> > + def_temp = gimple_build_debug_source_bind (vexpr, parm, >> > + NULL); >> > + gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT); >> > + def_temp = gimple_build_debug_bind (var, vexpr, NULL); >> > + gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT); >> > + } >> > + while (i); >> > + pop_cfun (); >> > + } >> > + >> > + t = VEC_index (tree, *debug_args, i + 1); >> > + gimple_debug_source_bind_set_value (stmt, t); >> > + stmt->gsbase.subcode = GIMPLE_DEBUG_BIND; >> >> That looks fishy ... shouldn't it be at least the other way around? >> >> stmt->gsbase.subcode = GIMPLE_DEBUG_BIND; >> gimple_debug_bind_set_value (stmt, t); > > It surely can. I was considering whether to create a new wrapper > in gimple.h for the subcode change of a debug stmt, but if there are > no further places needing this (don't see them right now), that might be > overkill. > >> Otherwise this looks ok. > > Thanks. > > Jakub
--- gcc/ipa-split.c.jj 2012-09-25 14:26:52.612821323 +0200 +++ gcc/ipa-split.c 2012-10-02 17:41:31.030357922 +0200 @@ -1059,6 +1059,7 @@ split_function (struct split_point *spli gimple last_stmt = NULL; unsigned int i; tree arg, ddef; + VEC(tree, gc) **debug_args = NULL; if (dump_file) { @@ -1232,6 +1233,65 @@ split_function (struct split_point *spli gimple_set_block (call, DECL_INITIAL (current_function_decl)); VEC_free (tree, heap, args_to_pass); + if (args_to_skip) + for (parm = DECL_ARGUMENTS (current_function_decl), num = 0; + parm; parm = DECL_CHAIN (parm), num++) + if (bitmap_bit_p (args_to_skip, num) + && is_gimple_reg (parm)) + { + tree ddecl; + gimple def_temp; + + arg = get_or_create_ssa_default_def (cfun, parm); + if (!MAY_HAVE_DEBUG_STMTS) + continue; + if (debug_args == NULL) + debug_args = decl_debug_args_insert (node->symbol.decl); + ddecl = make_node (DEBUG_EXPR_DECL); + DECL_ARTIFICIAL (ddecl) = 1; + TREE_TYPE (ddecl) = TREE_TYPE (parm); + DECL_MODE (ddecl) = DECL_MODE (parm); + VEC_safe_push (tree, gc, *debug_args, DECL_ORIGIN (parm)); + VEC_safe_push (tree, gc, *debug_args, ddecl); + def_temp = gimple_build_debug_bind (ddecl, unshare_expr (arg), + call); + gsi_insert_after (&gsi, def_temp, GSI_NEW_STMT); + } + if (debug_args != NULL) + { + unsigned int i; + tree var, vexpr; + gimple_stmt_iterator cgsi; + gimple def_temp; + + push_cfun (DECL_STRUCT_FUNCTION (node->symbol.decl)); + var = BLOCK_VARS (DECL_INITIAL (node->symbol.decl)); + i = VEC_length (tree, *debug_args); + cgsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR)); + do + { + i -= 2; + while (var != NULL_TREE + && DECL_ABSTRACT_ORIGIN (var) + != VEC_index (tree, *debug_args, i)) + var = TREE_CHAIN (var); + if (var == NULL_TREE) + break; + vexpr = make_node (DEBUG_EXPR_DECL); + parm = VEC_index (tree, *debug_args, i); + DECL_ARTIFICIAL (vexpr) = 1; + TREE_TYPE (vexpr) = TREE_TYPE (parm); + DECL_MODE (vexpr) = DECL_MODE (parm); + def_temp = gimple_build_debug_source_bind (vexpr, parm, + NULL); + gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT); + def_temp = gimple_build_debug_bind (var, vexpr, NULL); + gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT); + } + while (i); + pop_cfun (); + } + /* We avoid address being taken on any variable used by split part, so return slot optimization is always possible. Moreover this is required to make DECL_BY_REFERENCE work. */ --- gcc/tree-inline.c.jj 2012-09-25 14:26:52.576821521 +0200 +++ gcc/tree-inline.c 2012-10-02 17:43:13.395786581 +0200 @@ -2374,6 +2374,31 @@ copy_debug_stmt (gimple stmt, copy_body_ gimple_debug_source_bind_set_var (stmt, t); walk_tree (gimple_debug_source_bind_get_value_ptr (stmt), remap_gimple_op_r, &wi, NULL); + /* When inlining and source bind refers to one of the optimized + away parameters, change the source bind into normal debug bind + referring to the corresponding DEBUG_EXPR_DECL that should have + been bound before the call stmt. */ + t = gimple_debug_source_bind_get_value (stmt); + if (t != NULL_TREE + && TREE_CODE (t) == PARM_DECL + && id->gimple_call) + { + VEC(tree, gc) **debug_args = decl_debug_args_lookup (id->src_fn); + unsigned int i; + if (debug_args != NULL) + { + for (i = 0; i < VEC_length (tree, *debug_args); i += 2) + if (VEC_index (tree, *debug_args, i) == DECL_ORIGIN (t) + && TREE_CODE (VEC_index (tree, *debug_args, i + 1)) + == DEBUG_EXPR_DECL) + { + t = VEC_index (tree, *debug_args, i + 1); + gimple_debug_source_bind_set_value (stmt, t); + stmt->gsbase.subcode = GIMPLE_DEBUG_BIND; + break; + } + } + } } processing_debug_stmt = 0; --- gcc/testsuite/gcc.dg/guality/pr54519-2.c.jj 2012-10-02 16:27:24.862658030 +0200 +++ gcc/testsuite/gcc.dg/guality/pr54519-2.c 2012-10-02 16:27:24.862658030 +0200 @@ -0,0 +1,45 @@ +/* PR debug/54519 */ +/* { dg-do run } */ +/* { dg-options "-g" } */ + +__attribute__((noinline, noclone)) void +fn1 (int x) +{ + __asm volatile ("" : "+r" (x) : : "memory"); +} + +static int +fn2 (int x, int y) +{ + if (y) + { + fn1 (x); /* { dg-final { gdb-test 17 "x" "6" } } */ + fn1 (x); /* { dg-final { gdb-test 17 "y" "25" } } */ + fn1 (x); + fn1 (x); + y = -2 + x; + y = y * y * y + y; + fn1 (x + y); /* { dg-final { gdb-test 22 "y" "68" } } */ + } + return x; +} + +__attribute__((noinline, noclone)) int +fn3 (int x, int y) +{ + return fn2 (x, y) + y; +} + +__attribute__((noinline, noclone)) int +fn4 (int x, int y) +{ + return fn2 (x, y) + y; +} + +int +main () +{ + fn3 (6, 25); + fn4 (4, 117); + return 0; +} --- gcc/testsuite/gcc.dg/guality/pr54519-1.c.jj 2012-10-02 16:27:24.862658030 +0200 +++ gcc/testsuite/gcc.dg/guality/pr54519-1.c 2012-10-02 16:27:24.862658030 +0200 @@ -0,0 +1,48 @@ +/* PR debug/54519 */ +/* { dg-do run } */ +/* { dg-options "-g" } */ + +__attribute__((noinline, noclone)) void +fn1 (int x) +{ + __asm volatile ("" : "+r" (x) : : "memory"); +} + +static int +fn2 (int x, int y, int z) +{ + int a = 8; + if (x != z) + { + fn1 (x); + fn1 (x); /* { dg-final { gdb-test 20 "x" "36" } } */ + if (x == 36) /* { dg-final { gdb-test 20 "y" "25" } } */ + fn1 (x); /* { dg-final { gdb-test 20 "z" "6" } } */ + fn1 (x); /* { dg-final { gdb-test 23 "x" "98" } } */ + if (x == 98) /* { dg-final { gdb-test 23 "y" "117" } } */ + fn1 (x); /* { dg-final { gdb-test 23 "z" "8" } } */ + fn1 (x); + fn1 (x + a); + } + return y; +} + +__attribute__((noinline, noclone)) int +fn3 (int x, int y) +{ + return fn2 (x, y, 6); +} + +__attribute__((noinline, noclone)) int +fn4 (int x, int y) +{ + return fn2 (x, y, 8); +} + +int +main () +{ + fn3 (36, 25); + fn4 (98, 117); + return 0; +} --- gcc/testsuite/gcc.dg/guality/pr54519-3.c.jj 2012-10-02 16:27:24.863658017 +0200 +++ gcc/testsuite/gcc.dg/guality/pr54519-3.c 2012-10-02 16:27:24.863658017 +0200 @@ -0,0 +1,42 @@ +/* PR debug/54519 */ +/* { dg-do run } */ +/* { dg-options "-g" } */ + +__attribute__((noinline, noclone)) void +fn1 (int x) +{ + __asm volatile ("" : "+r" (x) : : "memory"); +} + +static int +fn2 (int x, int y, int z) +{ + int a = 8; + if (x != z) + { + fn1 (x); + fn1 (x); /* { dg-final { gdb-test 20 "x" "36" } } */ + if (x == 36) /* { dg-final { gdb-test 20 "y" "25" } } */ + fn1 (x); /* { dg-final { gdb-test 20 "z" "6" } } */ + fn1 (x); /* { dg-final { gdb-test 23 "x" "98" } } */ + if (x == 98) /* { dg-final { gdb-test 23 "y" "117" } } */ + fn1 (x); /* { dg-final { gdb-test 23 "z" "8" } } */ + fn1 (x); + fn1 (x + a); + } + return y; +} + +int (*p) (int, int, int) = fn2; + +int +main () +{ + __asm volatile ("" : : : "memory"); + int (*q) (int, int, int) = p; + __asm volatile ("" : : : "memory"); + q (36, 25, 6); + q (98, 117, 8); + q (0, 0, 0); + return 0; +} --- gcc/testsuite/gcc.dg/guality/pr54519-4.c.jj 2012-10-02 16:27:24.863658017 +0200 +++ gcc/testsuite/gcc.dg/guality/pr54519-4.c 2012-10-02 16:27:24.863658017 +0200 @@ -0,0 +1,39 @@ +/* PR debug/54519 */ +/* { dg-do run } */ +/* { dg-options "-g" } */ + +__attribute__((noinline, noclone)) void +fn1 (int x) +{ + __asm volatile ("" : "+r" (x) : : "memory"); +} + +static int +fn2 (int x, int y) +{ + if (y) + { + fn1 (x); /* { dg-final { gdb-test 17 "x" "6" } } */ + fn1 (x); /* { dg-final { gdb-test 17 "y" "25" } } */ + fn1 (x); + fn1 (x); + y = -2 + x; + y = y * y * y + y; + fn1 (x + y); /* { dg-final { gdb-test 22 "y" "68" } } */ + } + return x; +} + +int (*p) (int, int) = fn2; + +int +main () +{ + __asm volatile ("" : : : "memory"); + int (*q) (int, int) = p; + __asm volatile ("" : : : "memory"); + q (6, 25); + q (4, 117); + q (0, 0); + return 0; +} --- gcc/testsuite/gcc.dg/guality/pr54519-5.c.jj 2012-10-02 16:27:24.863658017 +0200 +++ gcc/testsuite/gcc.dg/guality/pr54519-5.c 2012-10-02 16:27:24.863658017 +0200 @@ -0,0 +1,45 @@ +/* PR debug/54519 */ +/* { dg-do run } */ +/* { dg-options "-g" } */ + +__attribute__((noinline, noclone)) void +fn1 (int x) +{ + __asm volatile ("" : "+r" (x) : : "memory"); +} + +static int +fn2 (int x, int y) +{ + if (y) + { + fn1 (x); /* { dg-final { gdb-test 17 "x" "6" } } */ + fn1 (x); /* { dg-final { gdb-test 17 "y" "25" } } */ + fn1 (x); + fn1 (x); + y = -2 + x; + y = y * y * y + y; + fn1 (x + y); /* { dg-final { gdb-test 22 "y" "68" } } */ + } + return x; +} + +__attribute__((noinline, noclone)) int +fn3 (int x, int y) +{ + return fn2 (x, y); +} + +__attribute__((noinline, noclone)) int +fn4 (int x, int y) +{ + return fn2 (x, y); +} + +int +main () +{ + fn3 (6, 25); + fn4 (4, 117); + return 0; +} --- gcc/testsuite/gcc.dg/guality/pr54519-6.c.jj 2012-10-02 18:03:45.137732997 +0200 +++ gcc/testsuite/gcc.dg/guality/pr54519-6.c 2012-10-02 18:03:19.000000000 +0200 @@ -0,0 +1,27 @@ +/* PR debug/54519 */ +/* { dg-do run } */ +/* { dg-options "-g" } */ + +#include "../nop.h" + +static inline void +f1 (int x, int y) +{ + asm volatile (NOP); /* { dg-final { gdb-test 11 "x" "2" } } */ + asm volatile (NOP); /* { dg-final { gdb-test 11 "y" "0" } } */ +} + +static inline void +f2 (int z) +{ + f1 (z, 0); + f1 (z, 1); +} + +int +main () +{ + f2 (2); + f2 (3); + return 0; +}