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

Thu Jul 14 16:22:56 PDT 2022

dblaikie added a comment.

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.

@aprantl @probinson you folks have any thoughts on this? I'm starting to lean towards this being: 1) move the current "optimized variables" subprogram list into the lexical scope 2) add other local things into that list 3) profit. Though adding a list/fiel dto every lexical scope feels a bit heavy - not sure if there's a good metric/benchmark to measure the impact of that on memory usage, bitcode size, etc. It's probably not that big in the grand scheme of things, though.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125693/new/

https://reviews.llvm.org/D125693