[InstCombine] Merge DebugLoc when speculatively hoisting store instruction

[twoh]

Summary

Along with https://reviews.llvm.org/D27804, debug locations need to be merged when hoisting store instructions as well. Not sure if just dropping debug locations would make more sense for this case, but as the branch instruction will have at least different discriminator with the hoisted store instruction, I think there will be no difference in practice.

[aprantl]

Code LGTM, testcase could be improved a bit (comment inline).

test/Transforms/SimplifyCFG/remove-debug-2.ll

4

These CHECKs could be improved by adding a little more context so it is more obvious what is being checked. This also makes it easier to update the test in the future.

[twoh]

Address comments on test from @aprantl. Thanks for the comments. Wonder if it looks better now.

[twoh]

Friendly ping @aprantl. If the new test looks good to you, I'll commit this patch. Thanks!

[aprantl]

test/Transforms/SimplifyCFG/remove-debug-2.ll

4

Sorry for the delay!
This is an improvement, but not what I had in mind. Typically, we check for a specific instruction and then match it up with the DILocation, so we can be sure that the location is attached to what we think it is.

e.g.

CHECK: define @bar
CHECK-NOT: ret
CHECK: void call @foo, !dbg ![[LOC1:[0-9]+]]

CHECK: ![[LOC1]] = DILocation(line: 2

[aprantl]

PS: feel free to commit at any time after fixing this.

[twoh]

Addressing comments from @aprantl. Thanks for the details comments!

lib/Transforms/Utils/SimplifyCFG.cpp

unsigned KnownIDs[] = {LLVMContext::MD_tbaa,
                            LLVMContext::MD_mem_parallel_loop_access};
     combineMetadata(I1, I2, KnownIDs);
 
     // I1 and I2 are being combined into a single instruction.  Its debug
     // location is the merged locations of the original instructions.
     if (!isa<CallInst>(I1))
       I1->setDebugLoc(
           DILocation::getMergedLocation(I1->getDebugLoc(), I2->getDebugLoc()));
- 
     I2->eraseFromParent();
     Changed = true;
 
     I1 = &*BB1_Itr++;
     I2 = &*BB2_Itr++;
     // Skip debug info if it is not identical.
     DbgInfoIntrinsic *DBI1 = dyn_cast<DbgInfoIntrinsic>(I1);
     DbgInfoIntrinsic *DBI2 = dyn_cast<DbgInfoIntrinsic>(I2);
static bool sinkLastInstruction(ArrayRef<BasicBlock*> Blocks) {
   if (!isa<StoreInst>(I0)) {
     auto *PNUse = dyn_cast<PHINode>(*I0->user_begin());
     if (!all_of(Insts, [&PNUse](const Instruction *I) -> bool {
           auto *U = cast<Instruction>(*I->user_begin());
           return U == PNUse;
         }))
       return false;
   }
-  
   // We don't need to do any more checking here; canSinkLastInstruction should
   // have done it all for us.
   SmallVector<Value*, 4> NewOperands;
   for (unsigned O = 0, E = I0->getNumOperands(); O != E; ++O) {
     // This check is different to that in canSinkLastInstruction. There, we
     // cared about the global view once simplifycfg (and instcombine) have
     // completed - it takes into account PHIs that become trivially
     // simplifiable.  However here we need a more local view; if an operand
void reset() {
         }
         Insts.push_back(Inst);
       }
     }
 
     bool isValid() const {
       return !Fail;
     }
-    
     void operator -- () {
       if (Fail)
         return;
       for (auto *&Inst : Insts) {
         for (Inst = Inst->getPrevNode(); Inst && isa<DbgInfoIntrinsic>(Inst);)
           Inst = Inst->getPrevNode();
         // Already at beginning of block.
         if (!Inst) {
if (isa<BranchInst>(T) && cast<BranchInst>(T)->isUnconditional())
       UnconditionalPreds.push_back(B);
     else if ((isa<BranchInst>(T) || isa<SwitchInst>(T)) && !Cond)
       Cond = T;
     else
       return false;
   }
   if (UnconditionalPreds.size() < 2)
     return false;
-  
   bool Changed = false;
   // We take a two-step approach to tail sinking. First we scan from the end of
   // each block upwards in lockstep. If the n'th instruction from the end of each
   // block can be sunk, those instructions are added to ValuesToSink and we
   // carry on. If we can sink an instruction but need to PHI-merge some operands
   // (because they're not identical in each instruction) we add these to
   // PHIOperands.
   unsigned ScanIdx = 0;
auto ProfitableToSinkInstruction = [&](LockstepReverseIterator &LRI) {
     for (auto *I : *LRI)
       for (auto *V : PHIOperands[I])
         if (InstructionsToSink.count(V) == 0)
           ++NumPHIdValues;
     DEBUG(dbgs() << "SINK: #phid values: " << NumPHIdValues << "\n");
     unsigned NumPHIInsts = NumPHIdValues / UnconditionalPreds.size();
     if ((NumPHIdValues % UnconditionalPreds.size()) != 0)
         NumPHIInsts++;
-    
     return NumPHIInsts <= 1;
   };
 
   if (ScanIdx > 0 && Cond) {
     // Check if we would actually sink anything first! This mutates the CFG and
     // adds an extra block. The goal in doing this is to allow instructions that
     // couldn't be sunk before to be sunk - obviously, speculatable instructions
     // (such as trunc, add) can be sunk and predicated already. So we check that
     // we're going to sink at least one non-speculatable instruction.
     LRI.reset();
     unsigned Idx = 0;
     bool Profitable = false;
     while (ProfitableToSinkInstruction(LRI) && Idx < ScanIdx) {
       if (!isSafeToSpeculativelyExecute((*LRI)[0])) {
         Profitable = true;
         break;
       }
       --LRI;
       ++Idx;
     }
     if (!Profitable)
       return false;
-    
     DEBUG(dbgs() << "SINK: Splitting edge\n");
     // We have a conditional edge and we're going to sink some instructions.
     // Insert a new block postdominating all blocks we're going to sink from.
     if (!SplitBlockPredecessors(BI1->getSuccessor(0), UnconditionalPreds,
                                 ".sink.split"))
       // Edges couldn't be split.
       return false;
     Changed = true;
   }
-  
   // Now that we've analyzed all potential sinking candidates, perform the
   // actual sink. We iteratively sink the last non-terminator of the source
   // blocks into their common successor unless doing so would require too
   // many PHI instructions to be generated (currently only one PHI is allowed
   // per sunk instruction).
   //
   // We can use InstructionsToSink to discount values needing PHI-merging that will
   // actually be sunk in a later iteration. This allows us to be more
for (unsigned SinkIdx = 0; SinkIdx != ScanIdx; ++SinkIdx) {
     // Because we've sunk every instruction in turn, the current instruction to
     // sink is always at index 0.
     LRI.reset();
     if (!ProfitableToSinkInstruction(LRI)) {
       // Too many PHIs would be created.
       DEBUG(dbgs() << "SINK: stopping here, too many PHIs would be created!\n");
       break;
     }
-    
     if (!sinkLastInstruction(UnconditionalPreds))
       return Changed;
     NumSinkCommons++;
     Changed = true;
   }
   return Changed;
 }
 
if (SpeculatedStoreValue) {
     IRBuilder<NoFolder> Builder(BI);
     Value *TrueV = SpeculatedStore->getValueOperand();
     Value *FalseV = SpeculatedStoreValue;
     if (Invert)
       std::swap(TrueV, FalseV);
     Value *S = Builder.CreateSelect(
         BrCond, TrueV, FalseV, TrueV->getName() + "." + FalseV->getName(), BI);
     SpeculatedStore->setOperand(0, S);
+    SpeculatedStore->setDebugLoc(
+        DILocation::getMergedLocation(
+          BI->getDebugLoc(), SpeculatedStore->getDebugLoc()));
   }
 
   // Metadata can be dependent on the condition we are hoisting above.
   // Conservatively strip all metadata on the instruction.
   for (auto &I : *ThenBB)
     I.dropUnknownNonDebugMetadata();
 
   // Hoist the instructions.

test/Transforms/SimplifyCFG/remove-debug-2.ll

+; RUN: opt < %s -simplifycfg -S | FileCheck %s
+
+; CHECK: DILocation(line: 2
+; CHECK: DILocation(line: 3
+; CHECK-NOT: DILocation(line: 4
+; CHECK: DILocation(line: 5
+;
+; Check if the debug info for hoisted "ret = 0" is removed
+;
+; int foo(int x) {
+;   int ret = 1;
+;   if (x)
+;     ret = 0;
+;   return ret;
+; }
+
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @foo(i32) !dbg !6 {
+  %2 = alloca i32, align 4
+  %3 = alloca i32, align 4
+  store i32 %0, i32* %2, align 4
+  store i32 1, i32* %3, align 4, !dbg !14
+  %4 = load i32, i32* %2, align 4, !dbg !15
+  %5 = icmp ne i32 %4, 0, !dbg !15
+  br i1 %5, label %6, label %7, !dbg !17
+
+; <label>:6:                                      ; preds = %1
+  store i32 0, i32* %3, align 4, !dbg !18
+  br label %7, !dbg !19
+
+; <label>:7:                                      ; preds = %6, %1
+  %8 = load i32, i32* %3, align 4, !dbg !20
+  ret i32 %8, !dbg !21
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "foo.c", directory: "b/")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{}
+!6 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !7, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!7 = !DISubroutineType(types: !8)
+!8 = !{!9, !9}
+!9 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!10 = !DILocalVariable(name: "x", arg: 1, scope: !6, file: !1, line: 1, type: !9)
+!11 = !DIExpression()
+!12 = !DILocation(line: 1, column: 13, scope: !6)
+!13 = !DILocalVariable(name: "ret", scope: !6, file: !1, line: 2, type: !9)
+!14 = !DILocation(line: 2, column: 7, scope: !6)
+!15 = !DILocation(line: 3, column: 7, scope: !16)
+!16 = distinct !DILexicalBlock(scope: !6, file: !1, line: 3, column: 7)
+!17 = !DILocation(line: 3, column: 7, scope: !6)
+!18 = !DILocation(line: 4, column: 9, scope: !16)
+!19 = !DILocation(line: 4, column: 5, scope: !16)
+!20 = !DILocation(line: 5, column: 10, scope: !6)
+!21 = !DILocation(line: 5, column: 3, scope: !6)