[PATCH] D144007: [DwarfDebug] Move emission of globals from beginModule() to endModule() (6/7)

Kristina Bessonova via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Mar 13 05:36:13 PDT 2023


krisb added a comment.

In D144007#4185548 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1333-1359
+/// Sort and unique GVEs by comparing their fragment offset.
+static SmallVectorImpl<DwarfCompileUnit::GlobalExpr> &
+sortGlobalExprs(SmallVectorImpl<DwarfCompileUnit::GlobalExpr> &GVEs) {
+  llvm::sort(
+      GVEs, [](DwarfCompileUnit::GlobalExpr A, DwarfCompileUnit::GlobalExpr B) {
+        // Sort order: first null exprs, then exprs without fragment
+        // info, then sort by fragment offset in bits.
----------------
dblaikie wrote:
> 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)
> 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144007



More information about the llvm-commits mailing list