[PATCH] D54997: [DebugInfo] PR39807: Create unknown locations when hoisting to locationless blocks

[Jeremy Morse via Phabricator via llvm-commits]

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


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1403
+                           : DILocation::getMergedLocation(I1->getDebugLoc(),
+                                                           I2->getDebugLoc()));
 
----------------
nikic wrote:
> jmorse wrote:
> > nikic wrote:
> > > The non-terminator case in L1330 uses `applyMergedLocation()`, which is basically what is done here, minus the attempt to use `InsertPt->getDebugLoc()` first.
> > > 
> > > I'm wondering why these two cases are handled differently. While using the debugloc of the insertion point is more correct than using the debugloc of an arbitrary (possibly dead) branch, it still does not seem correct either. Shouldn't we just always use the result of merging debug locs here?
> > Good question -- my impression is that adopting from the insertion point increases the probability of salvaging earlier locations, which might be optimised out after this transformation. That's not an immensely robust argument though, I'd appreciate additional opinions.
> After looking at D53390 more closely, I think the motivation there was not so much the debug loc of the terminator itself, but rather the debug loc of the selects that are inserted for phis in the successors, and whose debug loc will be by default derived from the builder insertion point `NT`.
> 
> I think these two don't necessarily have to be the same. The terminator debug loc can be the merged loc, while the select debug loc can use the InsertPt debug loc, which is likely going to be the debug loc of the select condition.
That makes sense, updated accordingly, the terminator now always gets a guaranteed (possibly unknown) location, while selects extend from InsertPt.

Given that conditional locations can no longer be inherited by the select insns (as NT would use a merged location), some of this logic could be dropped. (Possibly better in a follow-up commit, as that'd involve updating the regression test for pr39187).


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

https://reviews.llvm.org/D54997