[PATCH] D125693: [DebugInfo] Support types, imports and static locals declared in a lexical block (3/5)

Kristina Bessonova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Jul 24 22:49:29 PDT 2022


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



More information about the cfe-commits mailing list