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. Thanks, Dehao > > Richard. > >> -cary
