[DebugInfo] Handle same locations in DILocation::getMergedLocation

[rob.lougher]

Summary

Revision 289661 introduced the function DILocation::getMergedLocation for merging of debug locations. At the time is was simply a stub which always returned no location. This patch modifies getMergedLocation to handle the case where the two locations are the same or can't be discriminated.

The patch:

• Changes getMergedLocation to use DILocation::canDiscriminate.
• Fixes DILocation::canDiscriminate to add checks for directory, column and discriminator.
• Updates SimplifyCFG HoistThenElseCodeToIf to remove redundant check.

The patch also adds an additional test to simplifycfg_sink_last_inst.ll - when the debug locations of the sunk instructions are the same we should now use that location for the common instruction.

OK to commit?

Thanks,
Rob.

[aprantl]

Added a couple of inline comments. LGTM with these changes applied.

include/llvm/IR/DebugInfoMetadata.h

1303

I would reorder these conditions so the most common mismatches come first.
Perhaps: Line, Column, Discriminator, Filename, Directory?

lib/Transforms/InstCombine/InstCombinePHI.cpp

32

This should be a separate NFC commit. No need to review it.

lib/Transforms/Utils/SimplifyCFG.cpp

1579

separate commit please

[rob.lougher]

In D28521#644148, @aprantl wrote:

Added a couple of inline comments. LGTM with these changes applied.

Thanks for the review! Changes made as requested and committed as 4 commits (one closing the review and 3 NFCs).

include/llvm/IR/DebugInfoMetadata.h

DILocalScope *getInlinedAtScope() const {
     return getScope();
   }
 
   /// Check whether this can be discriminated from another location.
   ///
   /// Check \c this can be discriminated from \c RHS in a linetable entry.
   /// Scope and inlined-at chains are not recorded in the linetable, so they
   /// cannot be used to distinguish basic blocks.
-  ///
-  /// The current implementation is weaker than it should be, since it just
-  /// checks filename and line.
-  ///
-  /// FIXME: Add a check for getDiscriminator().
-  /// FIXME: Add a check for getColumn().
-  /// FIXME: Change the getFilename() check to getFile() (or add one for
-  /// getDirectory()).
   bool canDiscriminate(const DILocation &RHS) const {
-    return getFilename() != RHS.getFilename() || getLine() != RHS.getLine();
+    return getDirectory() != RHS.getDirectory() ||
+           getFilename() != RHS.getFilename() ||
+           getLine() != RHS.getLine() ||
+           getColumn() != RHS.getColumn() ||
+           getDiscriminator() != RHS.getDiscriminator();
   }
 
   /// Get the DWARF discriminator.
   ///
   /// DWARF discriminators distinguish identical file locations between
   /// instructions that are on different basic blocks.
   inline unsigned getDiscriminator() const;
 
   /// Returns a new DILocation with updated \p Discriminator.
   inline DILocation *cloneWithDiscriminator(unsigned Discriminator) const;
 
   /// When two instructions are combined into a single instruction we also
   /// need to combine the original locations into a single location.
   ///
   /// When the locations are the same we can use either location. When they
   /// differ, we need a third location which is distinct from either. If
   /// they have the same file/line but have a different discriminator we
   /// could create a location with a new discriminator. If they are from
   /// different files/lines the location is ambiguous and can't be
   /// represented in a single line entry.  In this case, no location
   /// should be set.
   ///
-  /// Currently this function is simply a stub, and no location will be
-  /// used for all cases.
-  static DILocation *getMergedLocation(const DILocation *LocA,
+  /// Currently the function does not create a new location. If the locations
+  /// are the same, or cannot be discriminated, the first location is returned.
+  /// Otherwise an empty location will be used.
+  static const DILocation *getMergedLocation(const DILocation *LocA,
                                              const DILocation *LocB) {
+    if (LocA && LocB && (LocA == LocB || !LocA->canDiscriminate(*LocB)))
+      return LocA;
     return nullptr;
   }
 
   Metadata *getRawScope() const { return getOperand(0); }
   Metadata *getRawInlinedAt() const {
     if (getNumOperands() == 2)
       return getOperand(1);
     return nullptr;

lib/Transforms/InstCombine/InstCombinePHI.cpp

 
 #define DEBUG_TYPE "instcombine"
 
 /// The PHI arguments will be folded into a single operation with a PHI node
 /// as input. The debug location of the single operation will be the merged
 /// locations of the original PHI node arguments.
 DebugLoc InstCombiner::PHIArgMergedDebugLoc(PHINode &PN) {
   auto *FirstInst = cast<Instruction>(PN.getIncomingValue(0));
-  DILocation *Loc = FirstInst->getDebugLoc();
+  const DILocation *Loc = FirstInst->getDebugLoc();
 
   for (unsigned i = 1; i != PN.getNumIncomingValues(); ++i) {
     auto *I = cast<Instruction>(PN.getIncomingValue(i));
     Loc = DILocation::getMergedLocation(Loc, I->getDebugLoc());
   }
 
   return Loc;
 }

lib/Transforms/Utils/SimplifyCFG.cpp

unsigned KnownIDs[] = {LLVMContext::MD_tbaa,
                            LLVMContext::MD_nonnull,
                            LLVMContext::MD_invariant_group,
                            LLVMContext::MD_align,
                            LLVMContext::MD_dereferenceable,
                            LLVMContext::MD_dereferenceable_or_null,
                            LLVMContext::MD_mem_parallel_loop_access};
     combineMetadata(I1, I2, KnownIDs);
 
-    // If the debug loc for I1 and I2 are different, as we are combining them
-    // into one instruction, we do not want to select debug loc randomly from 
-    // I1 or I2.
-    if (!isa<CallInst>(I1) &&  I1->getDebugLoc() != I2->getDebugLoc())
+    // 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++;
static bool sinkLastInstruction(ArrayRef<BasicBlock*> Blocks) {
   // and move it to the start of the successor block.
   for (unsigned O = 0, E = I0->getNumOperands(); O != E; ++O)
     I0->getOperandUse(O).set(NewOperands[O]);
   I0->moveBefore(&*BBEnd->getFirstInsertionPt());
 
   // The debug location for the "common" instruction is the merged locations of
   // all the commoned instructions.  We start with the original location of the
   // "common" instruction and iteratively merge each location in the loop below.
-  DILocation *Loc = I0->getDebugLoc();
+  const DILocation *Loc = I0->getDebugLoc();
 
   // Update metadata and IR flags, and merge debug locations.
   for (auto *I : Insts)
     if (I != I0) {
       Loc = DILocation::getMergedLocation(Loc, I->getDebugLoc());
       combineMetadataForCSE(I0, I);
       I0->andIRFlags(I);
     }

test/DebugInfo/Generic/simplifycfg_sink_last_inst.ll

if.else:                                          ; preds = %entry
   %sub2 = sub nsw i32 %b, %call1, !dbg !13
   br label %if.end
 
 if.end:                                           ; preds = %if.else, %if.then
   %b.addr.0 = phi i32 [ %sub, %if.then ], [ %sub2, %if.else ]
   ret i32 %b.addr.0, !dbg !14
 }
 
+; When the commoned instructions have the same debug location, this location
+; should be used as the location of the common instruction.
+
+; Generated from source (with -mllvm -no-discriminators and -gno-column-info):
+
+; int test2(int a, int b) {
+;   if(a) b -= foo(); else b -= bar();
+;   return b;
+; }
+
+; CHECK: define i32 @test2
+; CHECK-LABEL: if.end:
+; CHECK: %[[PHI:.*]] = phi i32 [ %call1, %if.else ], [ %call, %if.then ]
+; CHECK: sub nsw i32 %b, %[[PHI]], !dbg ![[DBG:.*]]
+; CHECK: ret i32
+; CHECK: ![[DBG]] = !DILocation(line: 17, scope: !{{.*}})
+
+define i32 @test2(i32 %a, i32 %b) !dbg !15 {
+entry:
+  %tobool = icmp ne i32 %a, 0, !dbg !16
+  br i1 %tobool, label %if.then, label %if.else, !dbg !16
+
+if.then:                                          ; preds = %entry
+  %call = call i32 @foo(), !dbg !16
+  %sub = sub nsw i32 %b, %call, !dbg !16
+  br label %if.end, !dbg !16
+
+if.else:                                          ; preds = %entry
+  %call1 = call i32 @bar(), !dbg !16
+  %sub2 = sub nsw i32 %b, %call1, !dbg !16
+  br label %if.end
+
+if.end:                                           ; preds = %if.else, %if.then
+  %b.addr.0 = phi i32 [ %sub, %if.then ], [ %sub2, %if.else ]
+  ret i32 %b.addr.0, !dbg !17
+}
+
 declare i32 @foo()
 declare i32 @bar()
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "", isOptimized: false, runtimeVersion: 0, emissionKind: LineTablesOnly, enums: !2)
 !1 = !DIFile(filename: "test.c", directory: "")
 !2 = !{}
 !3 = !{i32 2, !"Dwarf Version", i32 4}
 !4 = !{i32 2, !"Debug Info Version", i32 3}
 !6 = distinct !DISubprogram(name: "test", scope: !1, file: !1, line: 8, type: !7, isLocal: false, isDefinition: true, scopeLine: 8, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
 !7 = !DISubroutineType(types: !2)
 !8 = !DILocation(line: 9, column: 6, scope: !6)
 !9 = !DILocation(line: 10, column: 10, scope: !6)
 !10 = !DILocation(line: 10, column: 7, scope: !6)
 !11 = !DILocation(line: 10, column: 5, scope: !6)
 !12 = !DILocation(line: 12, column: 10, scope: !6)
 !13 = !DILocation(line: 12, column: 7, scope: !6)
 !14 = !DILocation(line: 13, column: 3, scope: !6)
+!15 = distinct !DISubprogram(name: "test2", scope: !1, file: !1, line: 16, type: !7, isLocal: false, isDefinition: true, scopeLine: 16, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!16 = !DILocation(line: 17, scope: !15)
+!17 = !DILocation(line: 18, scope: !15)