[DebugInfo] Remove now un-necessary logic from HoistThenElseCodeToIf

[jmorse]

Summary

tl;dr: minor cleanup of line number computation to remove un-necessary complexity and avoid keeping dead line numbers alive.

This is a follow-up to D54997, where we saw that applyMergedLocation was required for correctly calculating debug locations when hoisting terminators. Using applyMergedLocation is correct, but makes the code necessary for fixing PR39187 [0,1] redundant: we now cannot leak debug locations from the hoisted blocks into the parent block via the terminator.

IMHO, when converting phis to selects in HoistThenElseCodeToIf we should remove the logic looking for prior non-debug instructions, and just inherit from the (merged) terminator location. The net effect is that code looking like this:

%10 = insn1...   # Line 1
%11 = insn2...   # Line 2
%12 = select...   # Line 2 (hoisted phi converted to select)
%13 = invoke @something to label %blah unwind label %fail   # Line 0

where the select inherited the line number from the insn above, will instead become:

%10 = insn1...   # Line 1
%11 = insn2...   # Line 2
%12 = select...   # Line 0 (hoisted phi converted to select)
%13 = invoke @something to label %blah unwind label %fail   # Line 0

where the line number comes from the hoisted terminator. There are two benefits:

• Simpler code in HoistThenElseCodeToIf
• Line numbers that get optimised away won't be kept alive through selects that appeared below them

I've adapted the original regression test to check for selects introduced by HoistThenElseCodeToIf getting an unknown location. The main point of the original PR was that they shouldn't get the line number from the hoisted terminator, which this behaviour honours.

This leaves the "getPrevNonDebugInstruction" methods without any users, but they seem like useful facilities to keep around.

[0] https://bugs.llvm.org/show_bug.cgi?id=39187
[1] https://reviews.llvm.org/D53390

[aprantl]

lib/Transforms/Utils/SimplifyCFG.cpp

1388 ↗

(On Diff #176629)

// PHIs created below will adopt NT's merged DebugLoc.

[jmorse]

Revise comment

[vsk]

LGTM. @CarlosAlbertoEnciso, any thoughts on this?

[probinson]

In D55272#1323317, @vsk wrote:

LGTM. @CarlosAlbertoEnciso, any thoughts on this?

FYI Carlos is away until the second week of January.

[jmorse]

NB, speaking directly to Carlos yesterday he approved of the cleanup. I'll likely land this today or Monday.

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

for (PHINode &PN : Succ->phis()) {
 
       if (isa<ConstantExpr>(BB1V) && !isSafeToSpeculativelyExecute(BB1V))
         return Changed;
       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 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);
   }
 
   // Ensure terminator gets a debug location, even an unknown one, in case
   // it involves inlinable calls.
   NT->applyMergedLocation(I1->getDebugLoc(), I2->getDebugLoc());
 
+  // PHIs created below will adopt NT's merged DebugLoc.
   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()) {

llvm/trunk/test/CodeGen/X86/pr39187-g.ll

 ; RUN: opt < %s -S -simplifycfg | FileCheck %s
 
 ; SimplifyCFG can hoist any common code in the 'then' and 'else' blocks to
 ; the 'if' basic block.
 ;
 ; For the special case, when hoisting the terminator instruction, its debug
 ; location keep references to its basic block, causing the debug information
 ; to become ambiguous. It causes the debugger to display unreached lines.
 
-; Change the debug location associated with the hoisted instruction, to
-; the debug location from the insertion point in the 'if' block.
-
-; The insertion point is the previous non-debug instruction before the
-; terminator in the parent basic block of the hoisted instruction.
-
-; IR with '-g':
-;
-;  [...]
-;  %frombool = zext i1 %cmp to i8, !dbg !26
-;  call void @llvm.dbg.value(metadata i8 %frombool, metadata !15, metadata !DIExpression()), !dbg !26
-;  call void @llvm.dbg.value(metadata i32 0, metadata !17, metadata !DIExpression()), !dbg !27
-;  br i1 %cmp, label %if.then, label %if.else
-;  [...]
-;
-; Insertion point is: %frombool = zext i1 %cmp to i8, !dbg !26
+; Check that hoisted instructions get unknown-location line numbers -- there
+; is no correct line number for code that has been common'd in this way.
 
 ; IR generated with:
 ; clang -S -g -gno-column-info -O2 -emit-llvm pr39187.cpp -o pr39187-g.ll -mllvm -opt-bisect-limit=10
 
 ; // pr39187.cpp
 ; int main() {
 ;   volatile int foo = 0;
 ;

 ; CHECK:  %face = alloca i8, align 1
 ; CHECK:  %foo.0..sroa_cast = bitcast i32* %foo to i8*
 ; CHECK:  store volatile i32 0, i32* %foo, align 4
 ; CHECK:  %foo.0. = load volatile i32, i32* %foo, align 4, !dbg !16
 ; CHECK:  %cmp = icmp eq i32 %foo.0., 4, !dbg !16
 ; CHECK:  %frombool = zext i1 %cmp to i8, !dbg !16
 ; CHECK:  call void @llvm.dbg.value(metadata i8 %frombool, metadata !13, metadata !DIExpression()), !dbg !16
 ; CHECK:  call void @llvm.dbg.value(metadata i32 0, metadata !15, metadata !DIExpression()), !dbg !17
-; CHECK:  %. = select i1 %cmp, i32 8, i32 4, !dbg !16
+; CHECK:  %. = select i1 %cmp, i32 8, i32 4, !dbg ![[MERGEDLOC:[0-9]+]]
+; CHECK:  ![[MERGEDLOC]] = !DILocation(line: 0, scope: !7)
 
 ; ModuleID = 'pr39187.cpp'
 source_filename = "pr39187.cpp"
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-linux-gnu"
 
 ; Function Attrs: norecurse nounwind uwtable
 define dso_local i32 @main() local_unnamed_addr #0 !dbg !7 {
 entry:
   %foo = alloca i32, align 4
   %face = alloca i8, align 1
   %foo.0..sroa_cast = bitcast i32* %foo to i8*
   store volatile i32 0, i32* %foo, align 4
   %foo.0. = load volatile i32, i32* %foo, align 4, !dbg !26
   %cmp = icmp eq i32 %foo.0., 4, !dbg !26
   %frombool = zext i1 %cmp to i8, !dbg !26
   call void @llvm.dbg.value(metadata i8 %frombool, metadata !15, metadata !DIExpression()), !dbg !26
   call void @llvm.dbg.value(metadata i32 0, metadata !17, metadata !DIExpression()), !dbg !27
   br i1 %cmp, label %if.then, label %if.else
 
 if.then:                                          ; preds = %entry
   call void @llvm.dbg.value(metadata i32 8, metadata !14, metadata !DIExpression()), !dbg !25
-  br label %if.end
+  br label %if.end, !dbg !25
 
 if.else:                                          ; preds = %entry
-  call void @llvm.dbg.value(metadata i32 4, metadata !14, metadata !DIExpression()), !dbg !25
-  br label %if.end
+  call void @llvm.dbg.value(metadata i32 4, metadata !14, metadata !DIExpression()), !dbg !27
+  br label %if.end, !dbg !27
 
 if.end:                                           ; preds = %if.else, %if.then
   %beards.0 = phi i32 [ 8, %if.then ], [ 4, %if.else ]
   store volatile i8 %frombool, i8* %face, align 1
   %face.0. = load volatile i8, i8* %face, align 1
   %0 = and i8 %face.0., 1
   %tobool3 = icmp eq i8 %0, 0
   %cond4 = select i1 %tobool3, i32 0, i32 %beards.0