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

[Felipe de Azevedo Piovezan via Phabricator via llvm-commits]

fdeazeve added a comment.

In D139217#3967447 <https://reviews.llvm.org/D139217#3967447>, @aprantl wrote:

> 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?

I had only run the Clang/LLVM/LLDB test suites, but I will try compiling a larger project.



================
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) {
----------------
aprantl wrote:
> should this be `static`?
Yes! Though this code is going to be removed in the new version


================
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);
----------------
aprantl wrote:
> 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.
> Can you make an assertion that the Node is distinct? Otherwise this would be dangerous.

The assert on line 73 is doing that (`assert(!N.isUniqued())`). This assert allo

> 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?

Agreed, this implementation is very similar to what the ValueMapper class does, so maybe it is more general than it needs to be.
Adding a method to DILexicalBlockBase (I'm assuming this is the class you meant?) seems straightforward.


================
Comment at: llvm/lib/IR/DebugLoc.cpp:114
+  DILocation *CachedResult = nullptr;
+
+  for (DILocation *Loc = RootLoc; Loc; Loc = Loc->getInlinedAt()) {
----------------
aprantl wrote:
> 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.
Fixed in the next version


================
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();
----------------
aprantl wrote:
> 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.
Will fix in the next version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139217