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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Many thanks for the reviews, all.



================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1403
+                           : DILocation::getMergedLocation(I1->getDebugLoc(),
+                                                           I2->getDebugLoc()));
 
----------------
nikic wrote:
> nikic wrote:
> > jmorse wrote:
> > > 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).
> > This code can now be simplified to `NT->applyMergedLocation(I1->getDebugLoc(), I2->getDebugLoc())`, which would also remove the need to include the DebugInfoMetadata.h header.
> > 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).
> 
> To clarify, is your suggestion here to drop the InsertPt-based logic entirely, and give the selects the same (either merged or invalid) debug loc as the terminator?
> To clarify, is your suggestion here to drop the InsertPt-based logic entirely, and give the selects the same (either merged or invalid) debug loc as the terminator?

Correct -- the primary problem in PR39187 was the appearance of conditional debug locs, unconditionally. Correctly merging the debug locs for the terminator prevents those locations leaking into the destination BB. There's no need for fiddling with the selects' debug locs now, except preserving behaviour for PR39187's regression test, which I'd prefer to edit as a separate commit.


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

https://reviews.llvm.org/D54997