[PATCH] D139217: [CodeExtractor] Correctly propagate scope information post extraction

[Adrian Prantl via Phabricator via llvm-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

This looks good to me now. I have a few suggestions for better names and comments, but I'm fine with the mechanics!



================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2119
 
+  void setScope(DIScope *Scope) {
+    assert(!isUniqued());
----------------
The other sibling classes would call this `replaceScope()`.


================
Comment at: llvm/lib/IR/DebugLoc.cpp:72
+/// recreating the chain with "NewSP" instead.
+static DIScope *replaceSubprogram(DILocalScope &RootScope, DISubprogram &NewSP,
+                                  LLVMContext &Ctx,
----------------
This function doesn't replace anything. What do you think of `reparentScopeChain`, `cloneScopesForSubprogram()`?


================
Comment at: llvm/lib/IR/DebugLoc.cpp:87
+
+  DIScope *UpdatedScope = CachedResult ? CachedResult : &NewSP;
+  for (DIScope *ScopeToUpdate : reverse(ScopeChain)) {
----------------
Maybe add a comment explaining what the entire loop does?


================
Comment at: llvm/lib/IR/DebugLoc.cpp:126
+
+  for (const DILocation *LocToUpdate : reverse(LocChain)) {
+    UpdatedLoc =
----------------
Top-level comment for this loop?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139217