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

[jmorse]

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.

[jmorse]

Update makes this clang-format compliant

[nikic]

lib/Transforms/Utils/SimplifyCFG.cpp

1403 ↗

(On Diff #175662)

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]

lib/Transforms/Utils/SimplifyCFG.cpp

1403 ↗

(On Diff #175662)

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]

lib/Transforms/Utils/SimplifyCFG.cpp

1403 ↗

(On Diff #175662)

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]

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

[jmorse]

lib/Transforms/Utils/SimplifyCFG.cpp

1403 ↗

(On Diff #175662)

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]

lib/Transforms/Utils/SimplifyCFG.cpp

1379 ↗

(On Diff #175686)

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

1403 ↗

(On Diff #175662)

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]

Simplify DebugLoc assignment, correct a comment

[jmorse]

lib/Transforms/Utils/SimplifyCFG.cpp

1398 ↗

(On Diff #175695)

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]

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

lib/Transforms/Utils/SimplifyCFG.cpp

1403 ↗

(On Diff #175662)

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?

[jmorse]

Many thanks for the reviews, all.

lib/Transforms/Utils/SimplifyCFG.cpp

1403 ↗

(On Diff #175662)

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.

llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp

for (PHINode &PN : Succ->phis()) {
       if (isa<ConstantExpr>(BB2V) && !isSafeToSpeculativelyExecute(BB2V))
         return Changed;
     }
   }
 
   // As the parent basic block terminator is a branch instruction which is
   // removed at the end of the current transformation, use its previous
   // non-debug instruction, as the reference insertion point, which will
-  // provide the debug location for the instruction being hoisted. For BBs
+  // provide the debug location for generated select instructions. For BBs
   // with only debug instructions, use an empty debug location.
   Instruction *InsertPt =
       BIParent->getTerminator()->getPrevNonDebugInstruction();
 
   // Okay, it is safe to hoist the terminator.
   Instruction *NT = I1->clone();
   BIParent->getInstList().insert(BI->getIterator(), NT);
   if (!NT->getType()->isVoidTy()) {
     I1->replaceAllUsesWith(NT);
     I2->replaceAllUsesWith(NT);
     NT->takeName(I1);
   }
 
-  // The instruction NT being hoisted, is the terminator for the true branch,
-  // with debug location (DILocation) within that branch. We can't retain
-  // its original debug location value, otherwise 'select' instructions that
-  // are created from any PHI nodes, will take its debug location, giving
-  // the impression that those 'select' instructions are in the true branch,
-  // causing incorrect stepping, affecting the debug experience.
-  NT->setDebugLoc(InsertPt ? InsertPt->getDebugLoc() : DebugLoc());
+  // Ensure terminator gets a debug location, even an unknown one, in case
+  // it involves inlinable calls.
+  NT->applyMergedLocation(I1->getDebugLoc(), I2->getDebugLoc());
 
   IRBuilder<NoFolder> Builder(NT);
+  // If an earlier instruction in this BB had a location, adopt it, otherwise
+  // clear debug locations.
+  Builder.SetCurrentDebugLocation(InsertPt ? InsertPt->getDebugLoc()
+                                           : DebugLoc());
+
   // Hoisting one of the terminators from our successor is a great thing.
   // Unfortunately, the successors of the if/else blocks may have PHI nodes in
   // them.  If they do, all PHI entries for BB1/BB2 must agree for all PHI
   // nodes, so we insert select instruction to compute the final result.
   std::map<std::pair<Value *, Value *>, SelectInst *> InsertedSelects;
   for (BasicBlock *Succ : successors(BB1)) {
     for (PHINode &PN : Succ->phis()) {
       Value *BB1V = PN.getIncomingValueForBlock(BB1);

llvm/trunk/test/Transforms/SimplifyCFG/pr39807.ll

+; RUN: opt -S -simplifycfg < %s | FileCheck %s
+
+declare void @personality()
+
+define void @test(i1 %b) personality void()* @personality !dbg !1 {
+; CHECK:      invoke void @inlinable()
+; CHECK-NEXT:    to label %success unwind label %failure, !dbg ![[DBGLOC:[0-9]+]]
+    br i1 %b, label %if, label %else
+
+if:
+    invoke void @inlinable()
+        to label %success unwind label %failure, !dbg !2
+
+else:
+    invoke void @inlinable()
+        to label %success unwind label %failure, !dbg !8
+
+success:
+    ret void
+
+failure:
+    landingpad {}
+        cleanup
+    ret void
+}
+
+define internal void @inlinable() !dbg !7 {
+    ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!4, !5, !6}
+
+; CHECK: ![[DBGLOC]] = !DILocation(line: 0
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, runtimeVersion: 0, file: !3)
+!1 = distinct !DISubprogram(name: "test", unit: !0)
+!2 = !DILocation(line: 2, scope: !1)
+!3 = !DIFile(filename: "foo", directory: ".")
+!4 = !{i32 2, !"Dwarf Version", i32 4}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = !{i32 1, !"wchar_size", i32 4}
+!7 = distinct !DISubprogram(name: "inlinable", unit: !0)
+!8 = !DILocation(line: 3, scope: !1)