This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jmorse on Nov 28 2018, 4:01 AM.

Details

Summary

This is a fix for https://bugs.llvm.org/show_bug.cgi?id=39807, where two inlineable calls are merged to a basic block with no debug locations.

We have to pick _a_ location to avoid later inlining failures, but there are none in the destination BB, and we possibly can't pick either of the locations to hand if they're reached conditionally.

Thus, whenever there are no destination BB locations, delegate to getMergedLocation: it'll produce an "unknown" location if all else fails.

This solution is slightly stronger than necessary, in that non-call instructions will get "unknown" locations when merged into an otherwise empty BB. IMHO this is a feature: such insns explicitly don't have a location, and get marked as such.

The test case is adapted from the PR and checks that a) we don't crash, and b) an unknown location is emitted.

Diff Detail

Event Timeline

jmorse created this revision.Nov 28 2018, 4:01 AM
jmorse updated this revision to Diff 175662.Nov 28 2018, 4:04 AM

Update makes this clang-format compliant

nikic added inline comments.Nov 28 2018, 4:12 AM
lib/Transforms/Utils/SimplifyCFG.cpp
1403

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?

jmorse marked an inline comment as done.Nov 28 2018, 4:30 AM
jmorse added inline comments.
lib/Transforms/Utils/SimplifyCFG.cpp
1403

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.

nikic added inline comments.Nov 28 2018, 5:51 AM
lib/Transforms/Utils/SimplifyCFG.cpp
1403

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.

jmorse updated this revision to Diff 175686.Nov 28 2018, 7:10 AM

Always assign merged location to merged terminator; update merged phi's to use the selected DebugLoc or nothing.

jmorse marked an inline comment as done.Nov 28 2018, 7:16 AM
jmorse added inline comments.
lib/Transforms/Utils/SimplifyCFG.cpp
1403

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).

nikic added inline comments.Nov 28 2018, 7:29 AM
lib/Transforms/Utils/SimplifyCFG.cpp
1379

nit: "for the instruction being hoisted" -> "for generated select instructions" or similar.

1403

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.

jmorse updated this revision to Diff 175695.Nov 28 2018, 7:48 AM

Simplify DebugLoc assignment, correct a comment

jmorse added inline comments.Nov 28 2018, 7:51 AM
lib/Transforms/Utils/SimplifyCFG.cpp
1407

I guess this comment should explain _why_ this is happening rather than _what_; unfortunately there's little justification for this behaviour now. (IMHO something else to address in a follow-up).

nikic accepted this revision.Nov 28 2018, 8:06 AM

LGTM, though maybe someone else would like to double-check if the chosen semantics make sense?

lib/Transforms/Utils/SimplifyCFG.cpp
1403

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?

This revision is now accepted and ready to land.Nov 28 2018, 8:06 AM
aprantl accepted this revision.Nov 28 2018, 8:50 AM

Many thanks for the reviews, all.

lib/Transforms/Utils/SimplifyCFG.cpp
1403

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.

This revision was automatically updated to reflect the committed changes.