[DwarfDebug] Move emission of globals from beginModule() to endModule() (5/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

This patch moves the emission of global variables from DwarfDebug::beginModule()
to DwarfDebug::endModule(). Effectively, this changes nothing but the order
of debug entities in the resulting DWARF.

This is needed to simplify DWARF emission in a context of proper support of
function-local static variables (which are globals in terms of LLVM IR)
which comes in the next patch (D144008), making all the globals handled in
a single place, together with other global and local entities.

[dblaikie]

I guess this one's a bit clearer than the type one about why changing the order is important for getting local entities correctly scoped - local static is a global variable for the debug info and IR, so we need to emit the scopes before we'll know where to put the globals, etc.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1333–1359

If this function remains identical, maybe save moving it around for a separate patch to reduce the size/possible changes in this patch?

(similarly, could /possibly/ refactor the globals loop into a function in one patch, move where the call is in another, then move the function around (if you like/to bring it closer to usage) in another - making it clear that moving the call site isn't changing any other behavior)

[krisb]

Apply comments and rebase.

[krisb]

In D144007#4185548, @dblaikie wrote:

I guess this one's a bit clearer than the type one about why changing the order is important for getting local entities correctly scoped - local static is a global variable for the debug info and IR, so we need to emit the scopes before we'll know where to put the globals, etc.

The motivation for this patch is mostly the same as for types:

• unify/simplify emission of local and non-local entities getting everything done in one place (in our case DwarfDebug::endModule()). Keeping 'non-local' globals emission in DwarfDebug::beginModule() (while 'local' globals should be emitted not earlier than DwarfDebug::endModule()) would make the implementation much more complicated as both 'local' and 'global' globals emission depends on GVMap.
• make sure IR/debug metadata would not be changed after DWARF entities get created.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1333–1359

If this function remains identical, maybe save moving it around for a separate patch to reduce the size/possible changes in this patch?

There are no changes in the function, so I'll move it in a separate patch.

(similarly, could /possibly/ refactor the globals loop into a function in one patch, move where the call is in another, then move the function around (if you like/to bring it closer to usage) in another - making it clear that moving the call site isn't changing any other behavior)

I'd rather extract all the global entities emission (including types and imports) into a separate function, since DwarfDebug::endModule() is quite large. But I'd prefer to do this after all the functional changes get done, otherwise it may cause additional noise and complicate review, testing, committing, and handling possible issues postcommit.

[dblaikie]

In D144007#4188854, @krisb wrote:

In D144007#4185548, @dblaikie wrote:

I guess this one's a bit clearer than the type one about why changing the order is important for getting local entities correctly scoped - local static is a global variable for the debug info and IR, so we need to emit the scopes before we'll know where to put the globals, etc.

The motivation for this patch is mostly the same as for types:

• unify/simplify emission of local and non-local entities getting everything done in one place (in our case DwarfDebug::endModule()). Keeping 'non-local' globals emission in DwarfDebug::beginModule() (while 'local' globals should be emitted not earlier than DwarfDebug::endModule()) would make the implementation much more complicated as both 'local' and 'global' globals emission depends on GVMap.
• make sure IR/debug metadata would not be changed after DWARF entities get created.

Fair enough - some tension between independent changes being independent-ish (so they can be reverted/root caused/etc separately if needed) and keeping things consistent. I'm not insisting on any reordering/separation here if you feel this is the best way to go.

[dzhidzhoev]

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

[dzhidzhoev]

Rebased.

[dzhidzhoev]

Rebased.

[dzhidzhoev]

Rebase

[jmmartinez]

Hi @dzhidzhoev !
What's missing to land the rest of the patches? I missed the last discussions and I saw you just rebased this one.