[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/DwarfCompileUnit.h:66
+  /// LocalScopeToDieMap - A map of all local scopes for this unit.
+  DenseMap<const DIScope *, DIE *> LocalScopeToDieMap;
 
----------------
ellis wrote:
> Does this need to map from `DIScope` (rather than `DILocalScope`) because it could take a `DICommonBlock`? If so maybe we should rename this map to remove the "Local".
> 
> 
> Or could we possibly make this a map from `DISubprogram`?
There should be subclasses of DILocalScope only. Will fix the key. Thank you for pointing to this!


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1077
+    if (isa<DICommonBlock>(S))
+      S = S->getScope();
+    if (const auto *LScope = dyn_cast_or_null<DILocalScope>(S))
----------------
ellis wrote:
> Can a `DICommonBlock` have a scope that is also a `DICommonBlock`?
I'm not a Fortran expert, but as far as I can understand, this shouldn't be possible according to the common block's semantic (https://j3-fortran.org/doc/year/18/18-007r1.pdf, p. 8.10.2 COMMON statement).
I didn't find any examples of usage of createCommonBlock(), but I guess, that DICommonBlock should only have a DISubprogram as a scope. If so, DICommonBlock can be made a DILocalScope subclass, but I'm not sure this is semantically correct from Fortran language perspective.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1213-1218
+  // Collect functions with inlined subprograms.
+  for (const auto &F : M->functions())
+    for (const BasicBlock &BB : F)
+      for (const Instruction &I : BB)
+        if (auto DbgLoc = I.getDebugLoc())
+          recordInlinedSubprogram(DbgLoc);
----------------
ellis wrote:
> This is really great. Previously in https://reviews.llvm.org/D113736 I recorded concrete functions, but that is limited because we aren't sure if there will be an abstract origin. Here we record exactly which subprograms have abstract origins. Also, thanks for including my tests from that diff!
It's nice to know whether the given SP will have an abstract origin or not, but unfortunately, this way of obtaining such information isn't fully reliable. The problem is that having some DILocations with inlinedAt field doesn't guarantee that the inlined/abstract LexicalScope will be created (especially in optimized code). So, I'm not sure this is the right way to go. Maybe I should avoid doing this and collect all the scope DIEs instead to have more chances to emit accurate dwarf.

> Also, thanks for including my tests from that diff!
It's me who should thank you for the tests :) By the way, do you plan to commit them separately? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113741