> On testcases like that from PR60243 CFG build is dominated by > assign_discriminators because it expands locations again and again > and this got more expensive over the time. > > Cary - can you explain the overall logic of assign_discriminators, > specifically why if the last stmt of a block has UNKNOWN_LOCATION > we don't need any discriminator? That last stmt inherits the last > location from a previous stmt with location. Also why > do we look at e->dest _last_ stmt in addition to the first stmt?
Sorry, it's been a long time since I looked at this. I think that test for UNKNOWN_LOCATION is just a way of punting on an unexpected condition; the rest of the function won't work unless we have a valid locus to start with. > If I understand correctly the assignment is done at CFG construction > time rather than location emission time because we want it done > on a representation close to source? So if the last stmt has > the same line then the first should have as well? As for why we look at last_stmt as well as first_stmt, that has to do with the way for loops are expanded. See my explanation here: https://gcc.gnu.org/ml/gcc-patches/2009-07/msg01450.html > Or, to ask a different question - why is this done so early on > a non-optimized CFG rather than late on RTL right before we > emit .loc directives? It has to be done early because we need to have discriminators assigned for the tree_profile pass, in order to use profile feedback from an earlier run. > It would be nice if you could expand the comment before > assign_discriminators in some way addressing the above. > > The patch below cuts the time spent in assign_discriminators > down by a factor of 2.5. There's obvious cleanup possibilities > for the pointer hash-table given we should rather embed the > int, int pair rather than allocating it on the heap. Couldn't > figure out the appropriate hash-traits quickly enough though. This looks good, except won't the hash table now compare equal for two different locations where the line is the same, but the file is not? In the Google branches, we improved discriminator assignment quite a bit by tracking per instruction instead of per basic block. Here's my original patch to do that: https://gcc.gnu.org/ml/gcc-patches/2009-11/msg00563.html Your improvement to hoist the expansion of locus_e would still be useful there. Unfortunately, I never had the chance to update that patch to preserve the discriminator info across LTO streaming, which is why it remained only in the Google branches. There were a few follow-on patches; the last backport to the google/gcc-4_9 branch combined most of them: https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00869.html and I think there was one more discriminator patch after that: https://gcc.gnu.org/ml/gcc-patches/2014-08/msg02671.html -cary
