krisb added a comment. In D125693#3653631 <https://reviews.llvm.org/D125693#3653631>, @dblaikie wrote:
> In D125693#3648942 <https://reviews.llvm.org/D125693#3648942>, @krisb wrote: > >> In D125693#3644029 <https://reviews.llvm.org/D125693#3644029>, @dblaikie >> wrote: >> >>> In D125693#3641742 <https://reviews.llvm.org/D125693#3641742>, @krisb wrote: >>> >>>> @dblaikie, could you please take a look at this and/or D113741 >>>> <https://reviews.llvm.org/D113741>? Do you see any ways to proceed? >>> >>> My concern with this direction is that anything that adds lists to the IR >>> metadata makes it difficult for that data to be dropped when it becomes >>> unused (the type graph, apart from the "retained types" on the CU, is >>> structured so that if a variable, say, gets optimized away, or a function >>> gets optimized away and its parameters along with it, the types get dropped >>> too - similarly with function descriptions, they aren't in a list (they >>> used to be) and are instead referenced from the `llvm::Function` ensuring >>> that if the function is optimized away entirely, the debug info for that >>> goes away too). Admittedly function-local things are handled somewhat >>> differently, for instance there is a list on the `DISubprogram` of the >>> local variables to ensure they are retained through optimizations so name >>> lookup does the right things at function-scope. So /maybe/ it's OK to move >>> in that direction here, but it might look more like that, add these other >>> function-local things to the `DISubprogram`-scoped list (rename the list to >>> generalize over more than just variables), rather than adding per-scope >>> lists? >> >> Initially, I made the dedicated per-scope list of local static >> vars/imports/types to make possible to remove unused entities if their >> parent scope gets optimized out. This doesn't fully resolve your concern >> about local types that might be emitted even if they are not used, but this >> makes the things a bit better. Moreover, such local entities as well as >> variables/labels are not going to be emitted to a final DWARF if their scope >> was optimized away. Currently, to emit any local entity we need its scope to >> have LexicalScope defined. If there are no DILocations in such a scope (so >> that it may be considered as optimized out), it will not have LexicalScope >> defined for it, thus no variables/labels/other local things will be emitted >> for it. So, if we are not going to change this design in a near future, it >> might be worse to consider switching local variables/labels to per-scope >> list as well. >> >> What about merging variables/labels/other entities to a single list, I fully >> support this idea (it would require some additional checks in the backend, >> but would look more consistent). > > Ah, fair point about implicitly dropping some debug info if it's in a dead > scope. & yeah, maybe moving the existing optimized local variables list into > a per-scope list might be a nice improvement regardless. Well, this isn't as good as I thought. We are skipping optimized scopes and entities belong to them iff the scope is concrete (either inline or out-of-line). But we never skip abstract scopes; instead we create LexicalScope's just before constructing an abstract tree (see `DwarfDebug::ensureAbstractEntityIsCreated()`) if there are some retained nodes in them (currently, local variables or labels). I was a bit confused by assert in `DwarfDebug::endFunctionImpl()`: assert(LScopes.getAbstractScopesList().size() == NumAbstractScopes && "ensureAbstractEntityIsCreated inserted abstract scopes"); It checks that `DwarfDebug::ensureAbstractEntityIsCreated()` doesn't create any scopes, but this check is for subprogram scopes only, it doesn't guard against creation of lexical block scope. And we do create LexicalScope's for DILexicalBlock if there is a retained node belong to it. I tried to restore the same behavior with per-scope retained nodes approach, but it significantly affects compile time (22.6% geomean on CTMark). So, I agree with your first suggestion to reuse retainedNodes field of DISuprogram to track other local entities. I'll update this patch soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125693/new/ https://reviews.llvm.org/D125693 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
