[PATCH] D37877: Update getMergedLocation to check the instruction type and merge properly.

Dehao Chen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 14 17:52:12 PDT 2017


danielcdh marked an inline comment as done.
danielcdh added inline comments.


================
Comment at: include/llvm/IR/DebugInfoMetadata.h:1426-1428
+  static const DILocation *getMergedLocation(
+      const DILocation *LocA, const DILocation *LocB,
+      const Instruction *Inst = nullptr);
----------------
dblaikie wrote:
> Perhaps the API should change to be less error prone (so it's not the responsibility of the caller to know to pass the Instruction if it might be a call).
> 
> Could this be "applyMergedLocation" and have it take the Instruction as the first parameter & modify its debug location in place? (are there callers that don't have the destination Instruction on-hand at the point where they want to make this call?)
We will also have MachineInstruction to call this.


================
Comment at: lib/IR/DebugInfo.cpp:680-687
+  for (DILocation *L = LocA->getInlinedAt(); L; L = L->getInlinedAt())
+    InlinedLocationsA.insert(L);
+  const DILocation *Result = LocB;
+  for (DILocation *L = LocB->getInlinedAt(); L; L = L->getInlinedAt()) {
+    Result = L;
+    if (InlinedLocationsA.count(L))
+      break;
----------------
dblaikie wrote:
> I'd expect to walk more than the inlined-at attributes, but the plain scopes too. Which should look something like "keep calling getScope until it returns null, then use getInlinedAt, then go back to walking scopes, etc, until getInlinedAt returns null".
> 
> Probably not as important/necessary, but could still help smooth out debug_ranges which cost a bunch of debug info size.
Not sure if I understand... why would walking scopes help? And also, the case for merged call should be very rare, may not have big impact on overall size?


https://reviews.llvm.org/D37877





More information about the llvm-commits mailing list