[DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (4/7)

[dzhidzhoev]

Summary

RFC https://discourse.llvm.org/t/rfc-dwarfdebug-fix-and-improve-handling-imported-entities-types-and-static-local-in-subprogram-and-lexical-block-scopes/68544

Similar to imported declarations, the patch tracks function-local types in
DISubprogram's 'retainedNodes' field. DwarfDebug is adjusted in accordance with
the aforementioned metadata change and provided a support of function-local
types scoped within a lexical block.

The patch assumes that DICompileUnit's 'enums field' no longer tracks local
types and DwarfDebug would assert if any locally-scoped types get placed there.

[jmmartinez]

Just a few minor comments. Everything else seems good to me.

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

728

I cannot find the link to PR55680. Would you mind sharing it?

You could also reference local-type-as-template-parameter.ll, your test depicts the issue very clearly.

730–731

NIT: You could use find to avoid searching in LexicalBlockDIEs twice.

743–745

NIT: You could use insert/try_emplace instead of using count and operator[]. The assertion would become something like:

auto Inserted = getAbstractScopeDIEs().try_emplace(DS, ScopeDIE);
assert(Inserted.second && "Abstract DIE for this scope exists!");
return ScopeDIE;

[krisb]

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

728

I cannot find the link to PR55680. Would you mind sharing it?

You could also reference local-type-as-template-parameter.ll, your test depicts the issue very clearly.

Here is the link to the issue https://github.com/llvm/llvm-project/issues/55680.
Mentioned the test in the comment. Thank you!

743–745

I'd slightly prefer to keep the original code as it looks a bit more readable to me (in your example, there should be another line to void out unused Inserted variable, otherwise it'd cause a warning).
Additional count() call in the assert() doesn't seem too expensive to me, but if you still think try_emplace() is better, I'll change to it.

[krisb]

Apply review comments and rebase.

[jmmartinez]

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

743–745

Ok for me. If the use of count was outside the assert I would have argued against.

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

743–745

FWIW I'd lean towards avoiding the extra lookup in the +Asserts build (by using emplace or try_emplace, etc - yeah, with the extra void cast for the non-asserts-unused variable), but wouldn't insist on it.

[dzhidzhoev]

See https://reviews.llvm.org/D143984#4341780

[dzhidzhoev]

Rebased.