Add discriminators for call instructions that are from the same line and same basic block.

[danielcdh]

Summary

Call instructions that are from the same line and same basic block needs to have separate discriminators to distinguish between different callsites.

[probinson]

Wouldn't you distinguish these by column number? Discriminator is intended to be for paths that can't be distinguished solely by source location.

[danielcdh]

In D14464#284194, @probinson wrote:

Wouldn't you distinguish these by column number? Discriminator is intended to be for paths that can't be distinguished solely by source location.

The main user of discriminator is AutoFDO, which does not use column number to index the profile for many reasons. So discriminator is used here to distinguish control flows that reside within the same line.

[dblaikie]

Even with column info, its insufficient if both calls came from the same
macro use:

#define X f(); f()
int main() {

X;

}

Both calls to 'f' will have the same line and column, the location of the X
use.

[probinson]

In D14464#284332, @dblaikie wrote:

Even with column info, its insufficient if both calls came from the same
macro use:

#define X f(); f()
int main() {

X;

}

Both calls to 'f' will have the same line and column, the location of the X
use.

Of course if the column numbers are the same, you need the discriminator. The provided test case did not have that situation, which is why I asked. You don't want to be chewing up extra space in the line table if you don't need to.

[danielcdh]

Sure, we can redesign the sample profile format to index using (line, column), but this will lead to:

• profile size would nearly double
• llvm will not be able to use gcc profile

I can do the experiment to measure the debug info impact of this patch. As discriminator does not happen quite often, so it will only add very small amount to the debug info size.

[dnovillo]

LGTM with a minor documentation fix.

lib/Transforms/Utils/AddDiscriminators.cpp

229

Add some explanation here as to why this is important for SamplePGO? Thanks.

[danielcdh]

FYI, with this patch, clang-3.8 binary (built with clang itself in release mode with "-g" c/c++ option) increased from 1096.9MB to 1097.5 MB (around 0.05% increase)

[danielcdh]

Add explaination how this would affect sample based FDO.

lib/Transforms/Utils/AddDiscriminators.cpp

for (unsigned I = 0; I < Last->getNumSuccessors(); ++I) {
                          << CurrentDIL->getDiscriminator() << *I1 << "\n");
           }
         }
         DEBUG(dbgs() << "\n");
         Changed = true;
       }
     }
   }
+
+  // Traverse all instructions and assign new discriminators to call
+  // instructions with the same lineno that are in the same basic block.
+  // Sample base profile needs to distinguish different function calls within
+  // a same source line for correct profile annotation.
+  for (BasicBlock &B : F) {
+    const DILocation *FirstDIL = NULL;
+    for (auto &I : B.getInstList()) {
+      CallInst *Current = dyn_cast<CallInst>(&I);
+      if (Current) {
+        DILocation *CurrentDIL = Current->getDebugLoc();
+        if (FirstDIL) {
+          if (CurrentDIL && CurrentDIL->getLine() == FirstDIL->getLine() &&
+              CurrentDIL->getFilename() == FirstDIL->getFilename()) {
+            auto *Scope = FirstDIL->getScope();
+            auto *File = Builder.createFile(FirstDIL->getFilename(),
+                                            Scope->getDirectory());
+            auto *NewScope = Builder.createLexicalBlockFile(
+                Scope, File, FirstDIL->computeNewDiscriminator());
+            Current->setDebugLoc(DILocation::get(
+                Ctx, CurrentDIL->getLine(), CurrentDIL->getColumn(), NewScope,
+                CurrentDIL->getInlinedAt()));
+            Changed = true;
+          } else {
+            FirstDIL = CurrentDIL;
+          }
+        } else {
+          FirstDIL = CurrentDIL;
+        }
+      }
+    }
+  }
   return Changed;
 }

test/Transforms/AddDiscriminators/call.ll

+; RUN: opt < %s -add-discriminators -S | FileCheck %s
+
+; Discriminator support for calls that are defined in one line:
+; #1 void bar();
+; #2
+; #3 void foo() {
+; #4  bar();bar()/*discriminator 1*/;bar()/*discriminator 2*/;
+; #5 }
+
+; Function Attrs: uwtable
+define void @_Z3foov() #0 {
+  call void @_Z3barv(), !dbg !10
+; CHECK:  call void @_Z3barv(), !dbg ![[CALL0:[0-9]+]]
+  call void @_Z3barv(), !dbg !11
+; CHECK:  call void @_Z3barv(), !dbg ![[CALL1:[0-9]+]]
+  call void @_Z3barv(), !dbg !12
+; CHECK:  call void @_Z3barv(), !dbg ![[CALL2:[0-9]+]]
+  ret void, !dbg !13
+}
+
+declare void @_Z3barv() #1
+
+attributes #0 = { uwtable "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "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 = { "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "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" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!7, !8}
+!llvm.ident = !{!9}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 3.8.0 (trunk 250915) (llvm/trunk 251830)", isOptimized: false, runtimeVersion: 0, emissionKind: 1, enums: !2, subprograms: !3)
+!1 = !DIFile(filename: "c.cc", directory: "/tmp")
+!2 = !{}
+!3 = !{!4}
+!4 = distinct !DISubprogram(name: "foo", linkageName: "_Z3foov", scope: !1, file: !1, line: 3, type: !5, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: false, variables: !2)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null}
+!7 = !{i32 2, !"Dwarf Version", i32 4}
+!8 = !{i32 2, !"Debug Info Version", i32 3}
+!9 = !{!"clang version 3.8.0 (trunk 250915) (llvm/trunk 251830)"}
+!10 = !DILocation(line: 4, column: 3, scope: !4)
+!11 = !DILocation(line: 4, column: 9, scope: !4)
+!12 = !DILocation(line: 4, column: 15, scope: !4)
+!13 = !DILocation(line: 5, column: 1, scope: !4)
+
+; CHECK: ![[CALL1]] = !DILocation(line: 4, column: 9, scope: ![[CALL1BLOCK:[0-9]+]])
+; CHECK: ![[CALL1BLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 1)
+; CHECK: ![[CALL2]] = !DILocation(line: 4, column: 15, scope: ![[CALL2BLOCK:[0-9]+]])
+; CHECK: ![[CALL2BLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 2)