Sounds reasonable. The patch is updated, bootstrapped and passed all regression test.
Dehao On Tue, May 21, 2013 at 5:34 AM, Richard Biener <richard.guent...@gmail.com> wrote: > On Fri, May 17, 2013 at 5:48 PM, Dehao Chen <de...@google.com> wrote: >> On Fri, May 17, 2013 at 1:22 AM, Richard Biener >> <richard.guent...@gmail.com> wrote: >>> On Wed, May 15, 2013 at 6:50 PM, Cary Coutant <ccout...@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
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; @@ -125,11 +125,11 @@ static void factor_computed_gotos (void); /* Edges. */ static void make_edges (void); +static void assign_discriminators (void); 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 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); @@ -231,6 +231,7 @@ build_gimple_cfg (gimple_seq seq) /* Create the edges of the flowgraph. */ discriminator_per_locus.create (13); make_edges (); + assign_discriminators (); cleanup_dead_labels (); discriminator_per_locus.dispose (); } @@ -690,11 +691,7 @@ make_edges (void) fallthru = true; if (fallthru) - { - make_edge (bb, bb->next_bb, EDGE_FALLTHRU); - if (last) - assign_discriminator (gimple_location (last), bb->next_bb); - } + make_edge (bb, bb->next_bb, EDGE_FALLTHRU); } if (root_omp_region) @@ -717,7 +714,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) { @@ -752,22 +750,37 @@ same_line_p (location_t locus1, location_t locus2) && filename_cmp (from.file, to.file) == 0); } -/* Assign a unique discriminator value to block BB if it begins at the same - LOCUS as its predecessor block. */ +/* Assign discriminators to each basic block. */ static void -assign_discriminator (location_t locus, basic_block bb) +assign_discriminators (void) { - gimple first_in_to_bb, last_in_to_bb; + basic_block bb; - if (locus == 0 || bb->discriminator != 0) - return; + FOR_EACH_BB (bb) + { + edge e; + edge_iterator ei; + gimple last = last_stmt (bb); + location_t locus = last ? gimple_location (last) : UNKNOWN_LOCATION; - 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 (locus == UNKNOWN_LOCATION) + continue; + + FOR_EACH_EDGE (e, ei, bb->succs) + { + gimple first = first_non_label_stmt (e->dest); + gimple last = last_stmt (e->dest); + if ((first && same_line_p (locus, gimple_location (first))) + || (last && same_line_p (locus, gimple_location (last)))) + { + if (e->dest->discriminator != 0 && bb->discriminator == 0) + bb->discriminator = next_discriminator_for_locus (locus); + else + e->dest->discriminator = next_discriminator_for_locus (locus); + } + } + } } /* Create the edges for a GIMPLE_COND starting at block BB. */ @@ -780,13 +793,10 @@ make_cond_expr_edges (basic_block bb) basic_block then_bb, else_bb; tree then_label, else_label; edge e; - location_t entry_locus; gcc_assert (entry); gcc_assert (gimple_code (entry) == GIMPLE_COND); - entry_locus = gimple_location (entry); - /* Entry basic blocks for each component. */ then_label = gimple_cond_true_label (entry); else_label = gimple_cond_false_label (entry); @@ -796,14 +806,10 @@ 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); e->goto_locus = gimple_location (then_stmt); e = make_edge (bb, else_bb, EDGE_FALSE_VALUE); if (e) - { - assign_discriminator (entry_locus, else_bb); - e->goto_locus = gimple_location (else_stmt); - } + e->goto_locus = gimple_location (else_stmt); /* We do not need the labels anymore. */ gimple_cond_set_true_label (entry, NULL_TREE); @@ -923,11 +929,8 @@ static void make_gimple_switch_edges (basic_block bb) { gimple entry = last_stmt (bb); - location_t entry_locus; size_t i, n; - entry_locus = gimple_location (entry); - n = gimple_switch_num_labels (entry); for (i = 0; i < n; ++i) @@ -935,7 +938,6 @@ make_gimple_switch_edges (basic_block bb) tree lab = CASE_LABEL (gimple_switch_label (entry, i)); basic_block label_bb = label_to_block (lab); make_edge (bb, label_bb, 0); - assign_discriminator (entry_locus, label_bb); } } @@ -1020,7 +1022,6 @@ 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); gsi_remove (&last, true); return; } @@ -1035,7 +1036,6 @@ static void make_gimple_asm_edges (basic_block bb) { gimple stmt = last_stmt (bb); - location_t stmt_loc = gimple_location (stmt); int i, n = gimple_asm_nlabels (stmt); for (i = 0; i < n; ++i) @@ -1043,7 +1043,6 @@ make_gimple_asm_edges (basic_block bb) tree label = TREE_VALUE (gimple_asm_label_op (stmt, i)); basic_block label_bb = label_to_block (label); make_edge (bb, label_bb, 0); - assign_discriminator (stmt_loc, label_bb); } } 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; +}