Add -debug-info-for-profiling to emit more debug info for sample pgo profile collection

[danielcdh]

Summary

SamplePGO binaries built with -gmlt to collect profile. The current -gmlt debug info is limited, and we need some additional info:

• start line of all subprograms
• linkage name of all subprograms
• standalone subprograms (functions that has neither inlined nor been inlined)

This patch adds these information to the -gmlt binary. The impact on speccpu2006 binary size (size increase comparing with -g0 binary, also includes data for -g binary, which does not change with this patch):

-gmlt(orig) -gmlt(patched) -g

433.milc 4.68% 5.40% 19.73%
444.namd 8.45% 8.93% 45.99%
447.dealII 97.43% 115.21% 374.89%
450.soplex 27.75% 31.88% 126.04%
453.povray 21.81% 26.16% 92.03%
470.lbm 0.60% 0.67% 1.96%
482.sphinx3 5.77% 6.47% 26.17%
400.perlbench 17.81% 19.43% 73.08%
401.bzip2 3.73% 3.92% 12.18%
403.gcc 31.75% 34.48% 122.75%
429.mcf 0.78% 0.88% 3.89%
445.gobmk 6.08% 7.92% 42.27%
456.hmmer 10.36% 11.25% 35.23%
458.sjeng 5.08% 5.42% 14.36%
462.libquantum 1.71% 1.96% 6.36%
464.h264ref 15.61% 16.56% 43.92%
471.omnetpp 11.93% 15.84% 60.09%
473.astar 3.11% 3.69% 14.18%
483.xalancbmk 56.29% 81.63% 353.22%
geomean 15.60% 18.30% 57.81%

Debug info size change for -gmlt binary with this patch:

433.milc 13.46%
444.namd 5.35%
447.dealII 18.21%
450.soplex 14.68%
453.povray 19.65%
470.lbm 6.03%
482.sphinx3 11.21%
400.perlbench 8.91%
401.bzip2 4.41%
403.gcc 8.56%
429.mcf 8.24%
445.gobmk 29.47%
456.hmmer 8.19%
458.sjeng 6.05%
462.libquantum 11.23%
464.h264ref 5.93%
471.omnetpp 31.89%
473.astar 16.20%
483.xalancbmk 44.62%
geomean 16.83%

[probinson]

I was under the impression that gmlt consumers would use the ELF symbol table to derive the mangled name of the subprogram, which also tells you its starting address. The line table gives you the source position. Do you need the subprogram DIEs for something else?

[danielcdh]

guard the debug info change under -fprofile-debug.

[danielcdh]

ping....

[echristo]

With Minimal and the ProfileDebug it seems that the function can be rearranged to make the logic a bit more clear. Also needs comments describing what we're looking for as far as profile debug. (The clang flag also needs documentation come to think of it.)

Can you look into Minimal plus profile debugging and see what we can unify here? Also coverage analysis I imagine.

Thanks!

-eric

[danielcdh]

add some comments

[danielcdh]

In D25434#614157, @echristo wrote:

With Minimal and the ProfileDebug it seems that the function can be rearranged to make the logic a bit more clear. Also needs comments describing what we're looking for as far as profile debug. (The clang flag also needs documentation come to think of it.)

Added some comment in the new version. For the clang flag documentation, do you mean the changes in include/clang/Driver/Options.td ? Or could you point to which file I should update to change the documentation? Thanks

Can you look into Minimal plus profile debugging and see what we can unify here? Also coverage analysis I imagine.

I'm not quite sure if I understand correctly. My understanding was that Minimal and ProfileDebug are orthogonal?

Thanks,
Dehao

Thanks!

-eric

[danielcdh]

update option name

[danielcdh]

Ping....

Thanks
Dehao

[dblaikie]

Hmm - maybe this shouldn't be a TargetOptions flag?

Should this be module flag (passed through metadata) so that with LTO object files individual preferences are respected? (some may request this extra info, some may not)

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1187–1190

Out of date comment (refers to previous possible name for the flag)

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

1181–1186

Another out of date comment

1181–1187

Can this "DebugInfoForProfiling" check be floated up to the caller? (rename the 'Minimal' flag parameter to something more descriptive for the behavior change that applies to both debug-info-for-profiling and otherwise?)

[danielcdh]

In D25434#640048, @dblaikie wrote:

Hmm - maybe this shouldn't be a TargetOptions flag?

Should this be module flag (passed through metadata) so that with LTO object files individual preferences are respected? (some may request this extra info, some may not)

I think this should be a binary attribute because it will change the discriminator algorithm to encode the duplication factor. This should be consistent across all modules.

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

1181–1187

Changed Minimal to SkipSPAttributes because there is no need to output additional attributes when DebugInfoForProfiling (thus cannot float it to caller). Does it sound OK?

[danielcdh]

update

[dblaikie]

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

1181–1187

Good name - but I meant remove the testing of DebugInfoForProfiling here, but check it at the call sites?

Or perhaps that doesn't work out - multiple call sites, etc & you want to apply it in more cases. It's a while since I wrote this.

If that's the case (that it doesn't make sense to put this on one or two call sites) - could perhaps collapse it at the start of the function with something like:

SkipSPAttributes |= Asm->TM.Options.DebugInfoForProfiling;

1197–1199

DebugInfoForProfiling shouldn't be checked here?

[danielcdh]

update

[danielcdh]

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

1181–1187

It's not about multiple-callsites, but rather the behavior is different between: "SkipSPAttributes = true" and "SkipSPAttributes = false, DebugInfoForProfiling=true" (as noted in the next comment).

I've refactored the code a little bit to make it more clear.

1197–1199

Right, it should not be checked here because even with -fdebug-info-for-profiling, we don't need the extra debug info which is emitted after this return.

[dblaikie]

Test coverage would be good - add a flag to llc to test this functionality?

[danielcdh]

add unittest

[danielcdh]

a flag and a unittest are added.

Thanks,
Dehao

[dblaikie]

Looks good to me - but might want to hold off committing this and the clang side and poke @echristo about it. Sounded like he wanted to talk through some of the design here.

[echristo]

I'm not a fan of the TargetOptions method, but it seems to work as a first step and we can figure out the rest later.

Thanks for your patience.

-eric

[danielcdh]

Thanks for the reviews!

include/llvm/Target/TargetOptions.h

TargetOptions()
           UnsafeFPMath(false), NoInfsFPMath(false), NoNaNsFPMath(false),
           NoTrappingFPMath(false),
           HonorSignDependentRoundingFPMathOption(false), NoZerosInBSS(false),
           GuaranteedTailCallOpt(false), StackAlignmentOverride(0),
           StackSymbolOrdering(true), EnableFastISel(false), UseInitArray(false),
           DisableIntegratedAS(false), CompressDebugSections(false),
           RelaxELFRelocations(false), FunctionSections(false),
           DataSections(false), UniqueSectionNames(true), TrapUnreachable(false),
-          EmulatedTLS(false), EnableIPRA(false),
+          EmulatedTLS(false), EnableIPRA(false), DebugInfoForProfiling(false),
           FloatABIType(FloatABI::Default),
           AllowFPOpFusion(FPOpFusion::Standard),
           ThreadModel(ThreadModel::POSIX),
           EABIVersion(EABI::Default), DebuggerTuning(DebuggerKind::Default),
           FPDenormalMode(FPDenormal::IEEE),
           ExceptionModel(ExceptionHandling::None) {}
 
     /// PrintMachineCode - This flag is enabled when the -print-machineinstrs
public:
 
     /// EmulatedTLS - This flag enables emulated TLS model, using emutls
     /// function in the runtime library..
     unsigned EmulatedTLS : 1;
 
     /// This flag enables InterProcedural Register Allocation (IPRA).
     unsigned EnableIPRA : 1;
 
+    /// This flag enables emitting extra debug info for sample profiling.
+    unsigned DebugInfoForProfiling : 1;
+
     /// FloatABIType - This setting is set by -float-abi=xxx option is specfied
     /// on the command line. This setting may either be Default, Soft, or Hard.
     /// Default selects the target's default behavior. Soft selects the ABI for
     /// software floating point, but does not indicate that FP hardware may not
     /// be used. Such a combination is unfortunately popular (e.g.
     /// arm-apple-darwin). Hard presumes that the normal FP ABI is used.
     FloatABI::ABIType FloatABIType;
 
return
     ARE_EQUAL(FloatABIType) &&
     ARE_EQUAL(AllowFPOpFusion) &&
     ARE_EQUAL(ThreadModel) &&
     ARE_EQUAL(EABIVersion) &&
     ARE_EQUAL(DebuggerTuning) &&
     ARE_EQUAL(FPDenormalMode) &&
     ARE_EQUAL(ExceptionModel) &&
     ARE_EQUAL(MCOptions) &&
-    ARE_EQUAL(EnableIPRA);
+    ARE_EQUAL(EnableIPRA) &&
+    ARE_EQUAL(DebugInfoForProfiling);
 #undef ARE_EQUAL
 }
 
 inline bool operator!=(const TargetOptions &LHS,
                        const TargetOptions &RHS) {
   return !(LHS == RHS);
 }
 
 } // End llvm namespace
 
 #endif

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

void DwarfDebug::endFunction(const MachineFunction *MF) {
 
   DenseSet<InlinedVariable> ProcessedVars;
   collectVariableInfo(TheCU, SP, ProcessedVars);
 
   // Add the range of this function to the list of ranges for the CU.
   TheCU.addRange(RangeSpan(Asm->getFunctionBegin(), Asm->getFunctionEnd()));
 
   // Under -gmlt, skip building the subprogram if there are no inlined
-  // subroutines inside it.
-  if (TheCU.getCUNode()->getEmissionKind() == DICompileUnit::LineTablesOnly &&
+  // subroutines inside it. But with -fdebug-info-for-profiling, the subprogram
+  // is still needed as we need its source location.
+  if (!Asm->TM.Options.DebugInfoForProfiling &&
+      TheCU.getCUNode()->getEmissionKind() == DICompileUnit::LineTablesOnly &&
       LScopes.getAbstractScopesList().empty() && !IsDarwin) {
     assert(InfoHolder.getScopeVariables().empty());
     assert(DbgValues.empty());
     // FIXME: This wouldn't be true in LTO with a -g (with inlining) CU followed
     // by a -gmlt CU. Add a test and remove this assertion.
     assert(AbstractVariables.empty());
     PrevLabel = nullptr;
     CurFn = nullptr;

lib/CodeGen/AsmPrinter/DwarfUnit.h

public:
   void addType(DIE &Entity, const DIType *Ty,
                dwarf::Attribute Attribute = dwarf::DW_AT_type);
 
   DIE *getOrCreateNameSpace(const DINamespace *NS);
   DIE *getOrCreateModule(const DIModule *M);
   DIE *getOrCreateSubprogramDIE(const DISubprogram *SP, bool Minimal = false);
 
   void applySubprogramAttributes(const DISubprogram *SP, DIE &SPDie,
-                                 bool Minimal = false);
+                                 bool SkipSPAttributes = false);
 
   /// Find existing DIE or create new DIE for the given type.
   DIE *getOrCreateTypeDIE(const MDNode *N);
 
   /// Get context owner's DIE.
   DIE *createTypeDIE(const DICompositeType *Ty);
 
   /// Get context owner's DIE.

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

bool DwarfUnit::applySubprogramDefinitionAttributes(const DISubprogram *SP,
 
   // Refer to the function declaration where all the other attributes will be
   // found.
   addDIEEntry(SPDie, dwarf::DW_AT_specification, *DeclDie);
   return true;
 }
 
 void DwarfUnit::applySubprogramAttributes(const DISubprogram *SP, DIE &SPDie,
-                                          bool Minimal) {
-  if (!Minimal)
+                                          bool SkipSPAttributes) {
+  // If -fdebug-info-for-profiling is enabled, need to emit the subprogram
+  // and its source location.
+  bool SkipSPSourceLocation = SkipSPAttributes &&
+                              !Asm->TM.Options.DebugInfoForProfiling;
+  if (!SkipSPSourceLocation)
     if (applySubprogramDefinitionAttributes(SP, SPDie))
       return;
 
   // Constructors and operators for anonymous aggregates do not have names.
   if (!SP->getName().empty())
     addString(SPDie, dwarf::DW_AT_name, SP->getName());
 
+  if (!SkipSPSourceLocation)
+    addSourceLine(SPDie, SP);
+
   // Skip the rest of the attributes under -gmlt to save space.
-  if (Minimal)
+  if (SkipSPAttributes)
     return;
 
-  addSourceLine(SPDie, SP);
-
   // Add the prototype if we have a prototype and we have a C like
   // language.
   uint16_t Language = getLanguage();
   if (SP->isPrototyped() &&
       (Language == dwarf::DW_LANG_C89 || Language == dwarf::DW_LANG_C99 ||
        Language == dwarf::DW_LANG_ObjC))
     addFlag(SPDie, dwarf::DW_AT_prototyped);
 

lib/Target/TargetMachine.cpp

 #include "llvm/Target/TargetLoweringObjectFile.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
 using namespace llvm;
 
 cl::opt<bool> EnableIPRA("enable-ipra", cl::init(false), cl::Hidden,
                          cl::desc("Enable interprocedural register allocation "
                                   "to reduce load/store at procedure calls."));
 
+cl::opt<bool> DebugInfoForProfiling(
+    "debug-info-for-profiling", cl::init(false), cl::Hidden,
+    cl::desc("Emit extra debug info to make sample profile more accurate."));
+
 //---------------------------------------------------------------------------
 // TargetMachine Class
 //
 
 TargetMachine::TargetMachine(const Target &T, StringRef DataLayoutString,
                              const Triple &TT, StringRef CPU, StringRef FS,
                              const TargetOptions &Options)
     : TheTarget(T), DL(DataLayoutString), TargetTriple(TT), TargetCPU(CPU),
       TargetFS(FS), AsmInfo(nullptr), MRI(nullptr), MII(nullptr), STI(nullptr),
       RequireStructuredCFG(false), Options(Options) {
   if (EnableIPRA.getNumOccurrences())
     this->Options.EnableIPRA = EnableIPRA;
+  if (DebugInfoForProfiling.getNumOccurrences())
+    this->Options.DebugInfoForProfiling = DebugInfoForProfiling;
 }
 
 TargetMachine::~TargetMachine() {
   delete AsmInfo;
   delete MRI;
   delete MII;
   delete STI;
 }

test/DebugInfo/Generic/gmlt.test

 ; REQUIRES: object-emission
 ; RUN: %llc_dwarf -O0 -filetype=obj < %S/../Inputs/gmlt.ll | llvm-dwarfdump - | FileCheck %S/../Inputs/gmlt.ll
+; RUN: %llc_dwarf -O0 -filetype=obj -debug-info-for-profiling < %S/../Inputs/gmlt.ll | llvm-dwarfdump - | FileCheck %S/../Inputs/gmlt.ll --check-prefixes=PROFILING
 
 ; There's a darwin specific test in X86/gmlt, so it's okay to XFAIL this here.
 ; XFAIL: darwin

test/DebugInfo/Inputs/gmlt.ll

 ; Make sure we don't have any other subprograms here (subprograms with no
 ; inlined subroutines are omitted by design to save space)
 
 ; CHECK-NOT: {{DW_TAG|DW_AT}}
 ; CHECK: NULL
 ; CHECK-NOT: {{DW_TAG|DW_AT}}
 ; CHECK: NULL
 
+; PROFILING: DW_TAG_subprogram
+; PROFILING: DW_AT_name {{.*}} "f1"
+; With -debug-info-for-profiling, we need to emit decl_file a-nd decl_line
+; of the subprogram.
+; PROFILING: DW_AT_decl_file
+; PROFILING: DW_AT_decl_line
 
 ; CHECK: .debug_ranges contents:
 
 ; ... some addresses (depends on platform (such as platforms with function
 ; reordering in the linker), and looks wonky on platforms with zero values
 ; written in relocation places (dumper needs to be fixed to read the
 ; relocations rather than interpret that as the end of a range list))