Tolerate negative offset when matching sample profile.

[danielcdh]

Summary

In some cases (as illustrated in the unittest), lineno can be less than the heade_lineno because the function body are included from some other files. In this case, offset will be negative. This patch makes clang still able to match the profile to IR in this situation.

[dnovillo]

lib/Transforms/IPO/SampleProfile.cpp

233 ↗

(On Diff #37921)

Could you factor out this offset computation and add a comment? It's not immediately obvious why the code is doing this saturation and it's used in several places. Also, what's guaranteeing that the saturation will be done on 16 bits?

[danielcdh]

Refactor.

[danielcdh]

lib/Transforms/IPO/SampleProfile.cpp

245–246 ↗

(On Diff #37940)

Done.

In create_llvm_prof, offset is only encoded in 16 bits.

[dnovillo]

LGTM with the additional check. Thanks.

lib/Transforms/IPO/SampleProfile.cpp

186 ↗

(On Diff #37940)

I would now assert that this is the case (ie, L - H fits in 16 bits). It's true that most input will come straight out of create_llvm_prof, but not all of it will. In particular, the offsets in the input profiles are specified to be 32-bit unsigned integers.

Alternatively, we could assert this in ProfileData/SampleProfReader.cpp itself. This needs to be done in all 3 readers. Perhaps that's a better place to assert this.

245–246 ↗

(On Diff #37940)

Sure, but we're sufficiently removed from create_llvm_prof here that it makes sense to remind the user :)

[danielcdh]

Add sanity check.

[danielcdh]

lib/Transforms/IPO/SampleProfile.cpp

186 ↗

(On Diff #37943)

Added sanity check for LineLocation in text and binary reader. For gcov reader, the reader itself use bit operation to set LineLocation, which ensures that it's always within 16 bits.

[danielcdh]

Update the discriminator assignment algorithm.

• If a scope has already been assigned a discriminator, do not reassign a nested discriminator for it.
• If the file and line both match, even if the column does not match, we should assign a new discriminator for the stmt.

[danielcdh]

supposed to create a new patch... please ignore the previous update sent.

new patch created in http://reviews.llvm.org/D14181

lib/Transforms/Utils/AddDiscriminators.cpp

 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "add-discriminators"
 
 namespace {
-  struct AddDiscriminators : public FunctionPass {
-    static char ID; // Pass identification, replacement for typeid
-    AddDiscriminators() : FunctionPass(ID) {
-      initializeAddDiscriminatorsPass(*PassRegistry::getPassRegistry());
-    }
 
-    bool runOnFunction(Function &F) override;
-  };
 }
 
 char AddDiscriminators::ID = 0;
 INITIALIZE_PASS_BEGIN(AddDiscriminators, "add-discriminators",
                       "Add DWARF path discriminators", false, false)
 INITIALIZE_PASS_END(AddDiscriminators, "add-discriminators",
                     "Add DWARF path discriminators", false, false)
 
 // Command line option to disable discriminator generation even in the
 // presence of debug information. This is only needed when debugging
 // debug info generation issues.
-static cl::opt<bool>
-NoDiscriminators("no-discriminators", cl::init(false),
-                 cl::desc("Disable generation of discriminator information."));
+static cl::opt<bool> NoDiscriminators(
+    "no-discriminators", cl::init(false),
 
 FunctionPass *llvm::createAddDiscriminatorsPass() {
   return new AddDiscriminators();
 }
 
 static bool hasDebugInfo(const Function &F) {
   NamedMDNode *CUNodes = F.getParent()->getNamedMetadata("llvm.dbg.cu");
   return CUNodes != nullptr;

 /// file and line location as I2. This new lexical block will have a
 /// different discriminator number than I1.
 bool AddDiscriminators::runOnFunction(Function &F) {
   // If the function has debug information, but the user has disabled
   // discriminators, do nothing.
   // Simlarly, if the function has no debug info, do nothing.
   // Finally, if this module is built with dwarf versions earlier than 4,
   // do nothing (discriminator support is a DWARF 4 feature).
-  if (NoDiscriminators ||
-      !hasDebugInfo(F) ||
+  if (NoDiscriminators || !hasDebugInfo(F) ||
       F.getParent()->getDwarfVersion() < 4)
     return false;
 
   bool Changed = false;
   Module *M = F.getParent();
   LLVMContext &Ctx = M->getContext();
   DIBuilder Builder(*M, /*AllowUnresolved*/ false);
 
   // Traverse all the blocks looking for instructions in different
   // blocks that are at the same file:line location.
   for (BasicBlock &B : F) {
     TerminatorInst *Last = B.getTerminator();
     const DILocation *LastDIL = Last->getDebugLoc();
     if (!LastDIL)
       continue;
 
     for (unsigned I = 0; I < Last->getNumSuccessors(); ++I) {
       BasicBlock *Succ = Last->getSuccessor(I);
       Instruction *First = Succ->getFirstNonPHIOrDbgOrLifetime();
       const DILocation *FirstDIL = First->getDebugLoc();
-      if (!FirstDIL)
+      if (!FirstDIL || FirstDIL->getDiscriminator())
         continue;
 
       // If the first instruction (First) of Succ is at the same file
       // location as B's last instruction (Last), add a new
       // discriminator for First's location and all the instructions
       // in Succ that share the same location with First.
       if (!FirstDIL->canDiscriminate(*LastDIL)) {
         // Create a new lexical scope and compute a new discriminator
for (unsigned I = 0; I < Last->getNumSuccessors(); ++I) {
             DILocation::get(Ctx, FirstDIL->getLine(), FirstDIL->getColumn(),
                             NewScope, FirstDIL->getInlinedAt());
         DebugLoc newDebugLoc = NewDIL;
 
         // Attach this new debug location to First and every
         // instruction following First that shares the same location.
         for (BasicBlock::iterator I1(*First), E1 = Succ->end(); I1 != E1;
              ++I1) {
-          if (I1->getDebugLoc().get() != FirstDIL)
-            break;
-          I1->setDebugLoc(newDebugLoc);
-          DEBUG(dbgs() << NewDIL->getFilename() << ":" << NewDIL->getLine()
-                       << ":" << NewDIL->getColumn() << ":"
+          const DILocation *CurrentDIL = I1->getDebugLoc();
+          if (CurrentDIL && CurrentDIL->getLine() == FirstDIL->getLine() &&
+              CurrentDIL->getFilename() == FirstDIL->getFilename()) {
+            I1->setDebugLoc(DILocation::get(Ctx, CurrentDIL->getLine(),
+                                            CurrentDIL->getColumn(), NewScope,
+                                            CurrentDIL->getInlinedAt()));
+            DEBUG(dbgs() << CurrentDIL->getFilename() << ":"
+                         << CurrentDIL->getLine() << ":"
+                         << CurrentDIL->getColumn() << ":"
                          << NewDIL->getDiscriminator() << *I1 << "\n");
           }
+        }
         DEBUG(dbgs() << "\n");
         Changed = true;
       }
     }
   }
   return Changed;
 }

test/Transforms/AddDiscriminators/oneline.ll

+; RUN: opt < %s -add-discriminators -S | FileCheck %s
+
+define i32 @_Z3fooi(i32 %i) #0 {
+  %1 = alloca i32, align 4
+  %2 = alloca i32, align 4
+  store i32 %i, i32* %2, align 4, !tbaa !13
+  call void @llvm.dbg.declare(metadata i32* %2, metadata !9, metadata !17), !dbg !18
+  %3 = load i32, i32* %2, align 4, !dbg !19, !tbaa !13
+  %4 = icmp eq i32 %3, 3, !dbg !21
+  br i1 %4, label %8, label %5, !dbg !22
+
+; <label>:5                                       ; preds = %0
+  %6 = load i32, i32* %2, align 4, !dbg !23, !tbaa !13
+; CHECK:  %6 = load i32, i32* %2, align 4, !dbg ![[THEN1:[0-9]+]],{{.*}}
+
+  %7 = icmp eq i32 %6, 5, !dbg !24
+; CHECK:  %7 = icmp eq i32 %6, 5, !dbg ![[THEN2:[0-9]+]]
+
+  br i1 %7, label %8, label %9, !dbg !25
+; CHECK:  br i1 %7, label %8, label %9, !dbg ![[THEN3:[0-9]+]]
+
+; <label>:8                                       ; preds = %5, %0
+  store i32 100, i32* %1, align 4, !dbg !26
+; CHECK: store i32 100, i32* %1, align 4, !dbg ![[ELSE:[0-9]+]]
+
+  br label %10, !dbg !26
+; CHECK: br label %10, !dbg ![[ELSE]]
+
+; <label>:9                                       ; preds = %5
+  store i32 99, i32* %1, align 4, !dbg !27
+; CHECK: store i32 99, i32* %1, align 4, !dbg ![[COMBINE:[0-9]+]]
+
+  br label %10, !dbg !27
+; CHECK: br label %10, !dbg ![[COMBINE]]
+
+; <label>:10                                      ; preds = %9, %8
+  %11 = load i32, i32* %1, align 4, !dbg !28
+  ret i32 %11, !dbg !28
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+attributes #0 = { nounwind uwtable "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!10, !11}
+!llvm.ident = !{!12}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 3.8.0 (trunk 250915)", isOptimized: true, runtimeVersion: 0, emissionKind: 1, enums: !2, subprograms: !3)
+!1 = !DIFile(filename: "a.cc", directory: "/usr/local/google/home/dehao/discr")
+!2 = !{}
+!3 = !{!4}
+!4 = distinct !DISubprogram(name: "foo", linkageName: "_Z3fooi", scope: !1, file: !1, line: 1, type: !5, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: true, function: i32 (i32)* @_Z3fooi, variables: !8)
+!5 = !DISubroutineType(types: !6)
+!6 = !{!7, !7}
+!7 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!8 = !{!9}
+!9 = !DILocalVariable(name: "i", arg: 1, scope: !4, file: !1, line: 1, type: !7)
+!10 = !{i32 2, !"Dwarf Version", i32 4}
+!11 = !{i32 2, !"Debug Info Version", i32 3}
+!12 = !{!"clang version 3.8.0 (trunk 250915)"}
+!13 = !{!14, !14, i64 0}
+!14 = !{!"int", !15, i64 0}
+!15 = !{!"omnipotent char", !16, i64 0}
+!16 = !{!"Simple C/C++ TBAA"}
+!17 = !DIExpression()
+!18 = !DILocation(line: 1, column: 13, scope: !4)
+!19 = !DILocation(line: 2, column: 7, scope: !20)
+!20 = distinct !DILexicalBlock(scope: !4, file: !1, line: 2, column: 7)
+!21 = !DILocation(line: 2, column: 9, scope: !20)
+!22 = !DILocation(line: 2, column: 14, scope: !20)
+!23 = !DILocation(line: 2, column: 17, scope: !20)
+!24 = !DILocation(line: 2, column: 19, scope: !20)
+!25 = !DILocation(line: 2, column: 7, scope: !4)
+!26 = !DILocation(line: 2, column: 25, scope: !20)
+!27 = !DILocation(line: 2, column: 42, scope: !20)
+!28 = !DILocation(line: 3, column: 1, scope: !4)
+
+; CHECK: ![[THEN1]] = !DILocation(line: 2, column: 17, scope: ![[THENBLOCK:[0-9]+]])
+; CHECK: ![[THENBLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 2)
+; CHECK: ![[THEN2]] = !DILocation(line: 2, column: 19, scope: ![[THENBLOCK]])
+; CHECK: ![[THEN3]] = !DILocation(line: 2, column: 7, scope: ![[THENBLOCK]])
+; CHECK: ![[ELSE]] = !DILocation(line: 2, column: 25, scope: ![[ELSEBLOCK:[0-9]+]])
+; CHECK: ![[ELSEBLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 1)
+; CHECK: ![[COMBINE]] = !DILocation(line: 2, column: 42, scope: ![[COMBINEBLOCK:[0-9]+]])
+; CHECK: ![[COMBINEBLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 3)