Message ID | CAO2gOZVtO8Ux9HdCaMuSSzS8Rxk2usyVDBo+ba9_ehT1POyF0w@mail.gmail.com |
---|---|
State | New |
Headers | show |
> gcc/ChangeLog: > > * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. > (locus_descrim_hasher::equal): Likewise. > (next_discriminator_for_locus): Likewise. > (assign_discriminator): Add return value. > (make_edges): Assign more discriminator if necessary. > (make_cond_expr_edges): Likewise. > (make_goto_expr_edges): Likewise. > > gcc/testsuite/ChangeLog: > > * gcc.dg/debug/dwarf2/discriminator.c: New Test. Looks OK to me, but I can't approve patches for tree-cfg.c. Two comments: (1) In the C++ conversion, it looks like someone misspelled "discrim" in "locus_descrim_hasher". (2) Is this worth fixing in trunk when we'll eventually switch to a per-instruction discriminator? -cary
On Wed, May 15, 2013 at 9:50 AM, Cary Coutant <ccoutant@google.com> wrote: >> gcc/ChangeLog: >> >> * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. >> (locus_descrim_hasher::equal): Likewise. >> (next_discriminator_for_locus): Likewise. >> (assign_discriminator): Add return value. >> (make_edges): Assign more discriminator if necessary. >> (make_cond_expr_edges): Likewise. >> (make_goto_expr_edges): Likewise. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.dg/debug/dwarf2/discriminator.c: New Test. > > Looks OK to me, but I can't approve patches for tree-cfg.c. > > Two comments: > > (1) In the C++ conversion, it looks like someone misspelled "discrim" > in "locus_descrim_hasher". > > (2) Is this worth fixing in trunk when we'll eventually switch to a > per-instruction discriminator? Yeah, the same problem exists for per-instruction discriminator. This patch needs to be backported to google-4_7 and 4_8 branches. Thanks, Dehao > > -cary
On Wed, May 15, 2013 at 6:50 PM, Cary Coutant <ccoutant@google.com> wrote: >> gcc/ChangeLog: >> >> * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. >> (locus_descrim_hasher::equal): Likewise. >> (next_discriminator_for_locus): Likewise. >> (assign_discriminator): Add return value. >> (make_edges): Assign more discriminator if necessary. >> (make_cond_expr_edges): Likewise. >> (make_goto_expr_edges): Likewise. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.dg/debug/dwarf2/discriminator.c: New Test. > > Looks OK to me, but I can't approve patches for tree-cfg.c. > > Two comments: > > (1) In the C++ conversion, it looks like someone misspelled "discrim" > in "locus_descrim_hasher". > > (2) Is this worth fixing in trunk when we'll eventually switch to a > per-instruction discriminator? > This patch fixes a common case where one line is distributed to 3 BBs, > but only 1 discriminator is assigned. As far as I can see the patch makes discriminators coarser (by hashing and comparing on LOCATION_LINE and not on the location). It also has the changes like - assign_discriminator (entry_locus, then_bb); + if (assign_discriminator (entry_locus, then_bb)) + assign_discriminator (entry_locus, bb); which is what the comment refers to? I think those changes are short-sighted because what happens if the 2nd assign_discriminator call has exactly the same issue? Especially with the make_goto_expr_edges change there may be multiple predecessors where I cannot see how the change is correct. Yes, the change changes something and thus may fix the testcase but it doesn't change things in a predictable way as far as I can see. So - the change to compare/hash LOCATION_LINE looks good to me, but the assign_discriminator change needs more explaining. Richard. > -cary
On Fri, May 17, 2013 at 1:22 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Wed, May 15, 2013 at 6:50 PM, Cary Coutant <ccoutant@google.com> wrote: >>> gcc/ChangeLog: >>> >>> * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. >>> (locus_descrim_hasher::equal): Likewise. >>> (next_discriminator_for_locus): Likewise. >>> (assign_discriminator): Add return value. >>> (make_edges): Assign more discriminator if necessary. >>> (make_cond_expr_edges): Likewise. >>> (make_goto_expr_edges): Likewise. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * gcc.dg/debug/dwarf2/discriminator.c: New Test. >> >> Looks OK to me, but I can't approve patches for tree-cfg.c. >> >> Two comments: >> >> (1) In the C++ conversion, it looks like someone misspelled "discrim" >> in "locus_descrim_hasher". >> >> (2) Is this worth fixing in trunk when we'll eventually switch to a >> per-instruction discriminator? > >> This patch fixes a common case where one line is distributed to 3 BBs, >> but only 1 discriminator is assigned. > > As far as I can see the patch makes discriminators coarser (by hashing > and comparing on LOCATION_LINE and not on the location). It also has > the changes like > > - assign_discriminator (entry_locus, then_bb); > + if (assign_discriminator (entry_locus, then_bb)) > + assign_discriminator (entry_locus, bb); > > which is what the comment refers to? I think those changes are short-sighted > because what happens if the 2nd assign_discriminator call has exactly the > same issue? Especially with the make_goto_expr_edges change there > may be multiple predecessors where I cannot see how the change is > correct. Yes, the change changes something and thus may fix the testcase > but it doesn't change things in a predictable way as far as I can see. > > So - the change to compare/hash LOCATION_LINE looks good to me, > but the assign_discriminator change needs more explaining. The new discriminator assignment algorithm is: 1 FOR_EACH_BB: 2 FOR_EACH_SUCC_EDGE: 3 if (same_line(bb, succ_bb)) 4 if (succ_bb has no discriminator) 5 succ_bb->discriminator = new_discriminator 6 else if (bb has no discriminator) 7 bb->discriminator = new_discriminator Because there are so many places to handle SUCC_EDGE, thus the logic line #3, #4 is embedded within assign_discriminator function, and the logic in line #6 is encoded as the return value of assign_discriminator. Thanks, Dehao > > Richard. > >> -cary
On Fri, May 17, 2013 at 5:48 PM, Dehao Chen <dehao@google.com> wrote: > On Fri, May 17, 2013 at 1:22 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Wed, May 15, 2013 at 6:50 PM, Cary Coutant <ccoutant@google.com> wrote: >>>> gcc/ChangeLog: >>>> >>>> * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. >>>> (locus_descrim_hasher::equal): Likewise. >>>> (next_discriminator_for_locus): Likewise. >>>> (assign_discriminator): Add return value. >>>> (make_edges): Assign more discriminator if necessary. >>>> (make_cond_expr_edges): Likewise. >>>> (make_goto_expr_edges): Likewise. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> * gcc.dg/debug/dwarf2/discriminator.c: New Test. >>> >>> Looks OK to me, but I can't approve patches for tree-cfg.c. >>> >>> Two comments: >>> >>> (1) In the C++ conversion, it looks like someone misspelled "discrim" >>> in "locus_descrim_hasher". >>> >>> (2) Is this worth fixing in trunk when we'll eventually switch to a >>> per-instruction discriminator? >> >>> This patch fixes a common case where one line is distributed to 3 BBs, >>> but only 1 discriminator is assigned. >> >> As far as I can see the patch makes discriminators coarser (by hashing >> and comparing on LOCATION_LINE and not on the location). It also has >> the changes like >> >> - assign_discriminator (entry_locus, then_bb); >> + if (assign_discriminator (entry_locus, then_bb)) >> + assign_discriminator (entry_locus, bb); >> >> which is what the comment refers to? I think those changes are short-sighted >> because what happens if the 2nd assign_discriminator call has exactly the >> same issue? Especially with the make_goto_expr_edges change there >> may be multiple predecessors where I cannot see how the change is >> correct. Yes, the change changes something and thus may fix the testcase >> but it doesn't change things in a predictable way as far as I can see. >> >> So - the change to compare/hash LOCATION_LINE looks good to me, >> but the assign_discriminator change needs more explaining. > > The new discriminator assignment algorithm is: > > 1 FOR_EACH_BB: > 2 FOR_EACH_SUCC_EDGE: > 3 if (same_line(bb, succ_bb)) > 4 if (succ_bb has no discriminator) > 5 succ_bb->discriminator = new_discriminator > 6 else if (bb has no discriminator) > 7 bb->discriminator = new_discriminator > > Because there are so many places to handle SUCC_EDGE, thus the logic > line #3, #4 is embedded within assign_discriminator function, and the > logic in line #6 is encoded as the return value of > assign_discriminator. Hmm, as of current the code is hardly readable while the above makes sense (well, apart from what happens if both succ and bb already have a discriminator). Can I make you refactor the current code to postpone discriminator assignment until after make_edges completed, thus, do it in a postprocessing step done exactly like outlined above? That way also the whole thing, including the currently global discriminator_per_locus can be localized into a single function. Thanks, Richard. > Thanks, > Dehao > >> >> Richard. >> >>> -cary
> gcc/ChangeLog: > > * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. > (locus_descrim_hasher::equal): Likewise. > (next_discriminator_for_locus): Likewise. > (assign_discriminator): Add return value. > (make_edges): Assign more discriminator if necessary. > (make_cond_expr_edges): Likewise. > (make_goto_expr_edges): Likewise. > > gcc/testsuite/ChangeLog: > > * gcc.dg/debug/dwarf2/discriminator.c: New Test. The above ChangeLog entries are good, but the ones you installed aren't (wrong gcc/ prefix, wrong location for the testsuite entry, several typos, etc). Please fix.
Index: gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c =================================================================== --- gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c (revision 0) +++ gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c (revision 0) @@ -0,0 +1,16 @@ +/* { dg-do compile { target { i?86-*-* x86_64-*-* } } } */ +/* { dg-options "-O0 -gdwarf-2" } */ +/* { dg-final { scan-assembler "loc \[0-9] 9 \[0-9]( is_stmt \[0-9])?\n" } } */ +/* { dg-final { scan-assembler "loc \[0-9] 9 \[0-9]( is_stmt \[0-9])? discriminator 2\n" } } */ +/* { dg-final { scan-assembler "loc \[0-9] 9 \[0-9]( is_stmt \[0-9])? discriminator 1\n" } } */ + +int foo(int n) { + int i, ret = 0; + for (i = 0; i < n; i++) { + if (i % 10 == 1) + ret++; + else + ret--; + } + return ret; +} Index: gcc/tree-cfg.c =================================================================== --- gcc/tree-cfg.c (revision 198891) +++ gcc/tree-cfg.c (working copy) @@ -105,7 +105,7 @@ struct locus_descrim_hasher : typed_free_remove <l inline hashval_t locus_descrim_hasher::hash (const value_type *item) { - return item->locus; + return LOCATION_LINE (item->locus); } /* Equality function for the locus-to-discriminator map. A and B @@ -114,7 +114,7 @@ locus_descrim_hasher::hash (const value_type *item inline bool locus_descrim_hasher::equal (const value_type *a, const compare_type *b) { - return a->locus == b->locus; + return LOCATION_LINE (a->locus) == LOCATION_LINE (b->locus); } static hash_table <locus_descrim_hasher> discriminator_per_locus; @@ -129,7 +129,7 @@ static void make_cond_expr_edges (basic_block); static void make_gimple_switch_edges (basic_block); static void make_goto_expr_edges (basic_block); static void make_gimple_asm_edges (basic_block); -static void assign_discriminator (location_t, basic_block); +static bool assign_discriminator (location_t, basic_block); static edge gimple_redirect_edge_and_branch (edge, basic_block); static edge gimple_try_redirect_by_replacing_jump (edge, basic_block); static unsigned int split_critical_edges (void); @@ -693,7 +693,8 @@ make_edges (void) { make_edge (bb, bb->next_bb, EDGE_FALLTHRU); if (last) - assign_discriminator (gimple_location (last), bb->next_bb); + if (assign_discriminator (gimple_location (last), bb->next_bb)) + assign_discriminator (gimple_location (last), bb); } } @@ -717,7 +718,8 @@ next_discriminator_for_locus (location_t locus) item.locus = locus; item.discriminator = 0; - slot = discriminator_per_locus.find_slot_with_hash (&item, locus, INSERT); + slot = discriminator_per_locus.find_slot_with_hash ( + &item, LOCATION_LINE (locus), INSERT); gcc_assert (slot); if (*slot == HTAB_EMPTY_ENTRY) { @@ -753,21 +755,29 @@ same_line_p (location_t locus1, location_t locus2) } /* Assign a unique discriminator value to block BB if it begins at the same - LOCUS as its predecessor block. */ + LOCUS as its pred/succ block. Return true if discriminator has already + been assigned for BB and needs to assign additional discriminator for + BB's predecessor. */ -static void +static bool assign_discriminator (location_t locus, basic_block bb) { gimple first_in_to_bb, last_in_to_bb; - if (locus == 0 || bb->discriminator != 0) - return; + if (locus == 0) + return false; first_in_to_bb = first_non_label_stmt (bb); last_in_to_bb = last_stmt (bb); if ((first_in_to_bb && same_line_p (locus, gimple_location (first_in_to_bb))) || (last_in_to_bb && same_line_p (locus, gimple_location (last_in_to_bb)))) - bb->discriminator = next_discriminator_for_locus (locus); + { + if (bb->discriminator != 0) + return true; + else + bb->discriminator = next_discriminator_for_locus (locus); + } + return false; } /* Create the edges for a GIMPLE_COND starting at block BB. */ @@ -796,12 +806,14 @@ make_cond_expr_edges (basic_block bb) else_stmt = first_stmt (else_bb); e = make_edge (bb, then_bb, EDGE_TRUE_VALUE); - assign_discriminator (entry_locus, then_bb); + if (assign_discriminator (entry_locus, then_bb)) + assign_discriminator (entry_locus, bb); e->goto_locus = gimple_location (then_stmt); e = make_edge (bb, else_bb, EDGE_FALSE_VALUE); if (e) { - assign_discriminator (entry_locus, else_bb); + if (assign_discriminator (entry_locus, else_bb)) + assign_discriminator (entry_locus, bb); e->goto_locus = gimple_location (else_stmt); } @@ -1020,7 +1032,8 @@ make_goto_expr_edges (basic_block bb) basic_block label_bb = label_to_block (dest); edge e = make_edge (bb, label_bb, EDGE_FALLTHRU); e->goto_locus = gimple_location (goto_t); - assign_discriminator (e->goto_locus, label_bb); + if (assign_discriminator (e->goto_locus, label_bb)) + assign_discriminator (e->goto_locus, bb); gsi_remove (&last, true); return; }