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
