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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

This is great! The old code was quite incorrect :-)

I have one possible suggestion for a cleaner/safer implementation of `replaceOperand()`.
Did you test this on something large, like Clang?



================
Comment at: llvm/lib/IR/DebugLoc.cpp:71
+/// Updates the operands of N, replacing OperandToReplace with Replacement.
+void replaceOperand(MDNode &N, Metadata *OperandToReplace,
+                    MDNode *Replacement) {
----------------
should this be `static`?


================
Comment at: llvm/lib/IR/DebugLoc.cpp:74
+  assert(!N.isUniqued() && "Expected distinct or temporary nodes");
+  for (auto Idx : seq(0u, N.getNumOperands())) {
+    Metadata *Old = N.getOperand(Idx);
----------------
This seems to be very brute force. How much more code would it be to Switch over getKind() or put a replaceScope() method on DILexicalScopeBase?

Can you make an assertion that the Node is `distinct`? Otherwise this would be dangerous.


================
Comment at: llvm/lib/IR/DebugLoc.cpp:114
+  DILocation *CachedResult = nullptr;
+
+  for (DILocation *Loc = RootLoc; Loc; Loc = Loc->getInlinedAt()) {
----------------
This might benefit from a comment.
IIUC, we collect a chain of not-yet processed inlined-at locations, which would be useful when processing increasingly deeper inlined instructions.


================
Comment at: llvm/lib/IR/DebugLoc.cpp:126
+    // If no cache hits, then back() is the end of the inline chain and we need
+    // to update its *scope* chain.
+    DILocation *LocToUpdate = LocChain.pop_back_val();
----------------
maybe also make clear this this means we are now in the DILocation belonging to the current subprogram and that that's why we need to reparent the scope into the new function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139217