[PATCH] D113741: [RFC][DwarfDebug][AsmPrinter] Support emitting function-local declaration for a lexical block

[Kristina Bessonova via Phabricator via llvm-commits]

krisb added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1408
+      }
+      return llvm::make_range(CUs.begin(), CUs.end());
+    }
----------------
dblaikie wrote:
> krisb wrote:
> > dblaikie wrote:
> > > Wouldn't this return a dangling reference, since CUs is local to this function?
> > Right. Second time 'make_range' makes me thinking it's creating a copy here. My bad. Fixed (might be not the best way & best naming, but should be ok for a patch that, likely, will be abandoned in favor of D125693). 
> > patch that, likely, will be abandoned in favor of D125693
> 
> Not sure, I think this patch might still be a good direction - without the IR changes necessary for D125693. Does mean local types that end up unreferenced (because their uses are optimized awya) could be eliminated, which is good and bad (it's something we favor for non-local types for size reasons, but arguably we try more to keep all the local names for scoping/shadowing reasons - though it's not a terribly consistent principle)
The main problem with this patch is that in some cases function-local declarations may be placed incorrectly, and moreover, their placement may not be deterministic, depending on order of functions, as an example.
And unfortunately, I do not see good options to fix/improve this w/o IR changes.

The problem comes from `collectLocalScopesWithDeclsFromCU()` and a necessity to predict which lexical blocks are non-empty and require to be emitted (so later we can place local declarations to them).

`collectLocalScopesWithDeclsFromCU()` fills in a map that is a member of DwarfCompileUnit and it does this iff a DwarfCompileUnit is created. So, LocalScopesWithLocalDecls describes only local scopes belong to this CU.

But if to talk about split-dwarf, a subprogram from one CU (source CU) may be inlined into another CU (destination CU). So, when we emit such a subprogram we will check LocalScopesWithLocalDecls of destination CU which knows nothing about local entities for the subprogram. Moreover, source CU for this subprogram may be never emitted (added `split-dwarf-local-import3.ll` to demonstrate this).

We'd make LocalScopesWithLocalDecls a member of DwarfDebug and fill it in in `beginModule()`, but this looks a kind of weird.

It'd be good also to address the issue described in `collectLocalScopesWithDeclsFromCU()` for //used/ types, but it'll require additional computations (like, iterating over all the instructions, find all the used types and record their scopes to be sure later we can place that type correctly).

I do not like all this workarounds including the necessity to track mapping between emitted subprogram DIE and its parent CU. The approach splits emission of local declarations between 3 steps and it's hard to maintain everything in sync. 

D125693 requires changes in IR, but proposes a far more cleaner emission flow. 
If IR changes from D125691 is something that should be strongly avoided (but I found multiple places where comments mention function-local import should be tracked by a subprogram, not by a compile unit), I'll fix/improve whatever I can for this patch, but I'd like to consider D125693 as a next (second) step then.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h:318
+  /// Maps subprogram to its containing DwarfCompileUnit(s).
+  DenseMap<const DISubprogram *, SmallPtrSet<DwarfCompileUnit *, 2>> SPCUsMap;
+
----------------
dblaikie wrote:
> Why does this map from one subprogram to multiple CUs? Shouldn't we be only placing local types/static variables in one CU (on the abstract origin SP, which should be singular)?
We are returning to the question about multiple CUs in a DWO we've discussed a couple months ago. It still isn't clear whether this case is valid/useful for someone (e.g. split-dwarf+FullLTO).  

I've temporarily added 'split-dwarf-local-import-fulllto.ll' test that demonstrates this case and backend's behavior before (see CHECK prefix) and after (CHECK-NEW prefix) this patch. (Basically, `llvm/test/DebugInfo/X86/split-dwarf-cross-unit-reference.ll` also tests this case).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113741