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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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);
----------------
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?)


================
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;
----------------
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.


================
Comment at: test/Transforms/SimplifyCFG/remove-debug.ll:5-12
 ; CHECK: line: 6
 ; CHECK-NOT: line: 7
-; CHECK: line: 8
-; CHECK: line: 9
+; CHECK-NOT: line: 8
+; CHECK-NOT: line: 9
 ; CHECK-NOT: line: 10
-; CHECK: line: 11
+; CHECK-NOT: line: 11
+; CHECK: line: 0
----------------
Might be worth checking the specific instructions that these lines apply to?


https://reviews.llvm.org/D37877