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

[Dehao Chen via Phabricator via llvm-commits]

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);
----------------
danielcdh wrote:
> 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.
Updated the API:

MachineInstruction should use the original API
Instruction should use the new API to pass in the Instruction *


================
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;
----------------
danielcdh wrote:
> 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?
Discussed offline with David. It is an overkill to add another level of complexity to traverse Scope chain because

* it will not either help the user of the tool (the purpose of merge location is to prevent "wrong location" to be the "right location", not to get the right location, which is impossible to get anyway.
* it will not affect debug info size much: our experiments on a very large binary shows that it only has 0.00008% impact on the debug_line size and 0.00003% impact on the debug_info size.


https://reviews.llvm.org/D37877