[PATCH] D85670: [Instruction] Add updateLocationAfterHoist helper

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:723-724
+    // The parent function has no scope. Absent a better alternative, create a
+    // line 0 location with the existing scope/inlinedAt info.
+    setDebugLoc(DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));
+}
----------------
This alternative seems a bit problematic, as it may lead to differences in location depending on the order of inlining, which might be problematic/volatile/unpredictable for developers? but maybe there's no good answer. The other option only seems to be "drop the location entirely" (I think that's correct in the case where it isn't in a function with debug info? IF it gets inlined into such a function, it'll get a location at that point?)


================
Comment at: llvm/lib/IR/DebugInfo.cpp:717
+    // Avoid making it look like the inlined callee was reached early.
+    setDebugLoc(DebugLoc::get(0, 0, SP));
+  else
----------------
vsk wrote:
> aprantl wrote:
> > Should we pick up a scope from the instruction before it to avoid having a hole in that scope's range?
> I'd argue that we shouldn't. In this case, I think we want to use a scope that conveys 'you could be anywhere in this function'. The scope from a nearby instruction could be more specific than that.
Yeah, this one's unfortunate - when merging, we can find the nearest common scope and that's at least somewhat meaningful. But when hoisting that's not so clearly correct. :/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85670