llvm-dwarfdump: Stop counting out-of-line subprogram in the "inlined functions" statistic.

[cmtice]

Summary

DW_TAG_subprogram DIEs should not be counted in the inlined function statistic. This also addresses the source variables count, as that uses the inlined function count in its calculations.

[aprantl]

test/tools/llvm-dwarfdump/X86/statistics.ll

21 ↗

(On Diff #185641)

According to the comment on line 20 "source variables" is expected to be "unique source variables"+"number of extra inlined instances". If we want to change this we should update the comment as well.

I'd expect this program to have GlobalConst, Global, constant, s, square::i, cube:i, cube::squared = 7 unique variables, and source variables should be one more than that because it also counts the inlined square::i inside of cube.

This obviously isn't documented very well :-(, but based on that do you think this looks more like a bug in the documentation or do you think the algorithm is wrong?

[cmtice]

aprantl@, you were right, my original change was not correctly updating the variable counts to include variables occuring in inlined instances of functions. I have update the patch to fix this issue. There were two additional changes needed: one to make sure we were counting variables for functions that had no inlined instances at all; and one to avoid double-counting member constants.

[cmtice]

Now we correctly count only real constant members as "constant members" and we count global variables with locations as part of the "vars with locs".

[aprantl]

I would recommend bumping the version number, too, since the results from earlier versions won't be comparable, right?

test/tools/llvm-dwarfdump/X86/stats-inlining-multi-cu.ll

7 ↗

(On Diff #185806)

Seems right.

9 ↗

(On Diff #185806)

Where do the extra two variables come from?

tools/llvm-dwarfdump/Statistics.cpp

139 ↗

(On Diff #185806)

That looks definitely correct.

[cmtice]

test/tools/llvm-dwarfdump/X86/stats-inlining-multi-cu.ll

9 ↗

(On Diff #185806)

There are two inlined instances, each of which has 3 variables (parameter T, i from the first lexical block, i from the second lexical block); function a has a local variable 'a'; function b has a local variable 'b'. Total = 8.

[cmtice]

This increases the version number, as requested.

[aprantl]

test/tools/llvm-dwarfdump/X86/stats-inlining-multi-cu.ll

9 ↗

(On Diff #185806)

Is this counting the template parameter T as a variable? That seems wrong to me. I think we should only count entities that could have a DW_AT_location in their DW_TAG.

[dblaikie]

test/tools/llvm-dwarfdump/X86/stats-inlining-multi-cu.ll

9 ↗

(On Diff #185806)

Pedantically, a DW_TAG_template_value_parameter can have a DW_AT_location, for instance.

(but yeah, I agree - a count of variables probably shouldn't include template (type, value or otherwise) parameters)

[cmtice]

I was mistaken before: we were never counting the template parameters. What was happening was that we were counting the variables in both inlined instances, and we were also counting the variables for a concrete, out-of-line instance of the inlined function, even though in this particular case there was no concrete out-of-line instance. I have updated the patch now to only count vars in DW_TAG_subprogram DIEs if the subprogram DIE has PC addresses. I believe this is the correct fix for this issue.

[aprantl]

tools/llvm-dwarfdump/Statistics.cpp

283 ↗

(On Diff #185885)

I think we still need the + Constants here or VarTotal can be smaller than VarWithLoc which would be unintuitive.

[cmtice]

It appears to me that on line 131 we are already adding/counting the ConstantMembers in FnStats.VarsInFunction, so adding "+ Constants" would be counting them twice. Alternatively we can change the code on line 131 to not include ConstantMembers...

[aprantl]

Ah, good catch. Makes sense!

thanks!

llvm/trunk/test/tools/llvm-dwarfdump/X86/statistics.ll

 ; RUN: llc -O0 %s -o - -filetype=obj \
 ; RUN:   | llvm-dwarfdump -statistics - | FileCheck %s
-; CHECK: "version":1
+; CHECK: "version":2
 
 ; int GlobalConst = 42;
 ; int Global;
 ;
 ; struct S {
 ;   static const int constant = 24;
 ; } s;
 ;

llvm/trunk/test/tools/llvm-dwarfdump/X86/stats-inlining-multi-cu.ll

 ; RUN: llc -O0 %s -o - -filetype=obj \
 ; RUN:   | llvm-dwarfdump -statistics - | FileCheck %s
 
 ; Test that abstract origins in multiple CUs are uniqued.
 
 ; CHECK:      "source functions":4,
-; CHECK-SAME: "inlined functions":5,
+; CHECK-SAME: "inlined functions":2,
 ; CHECK-SAME: "unique source variables":4
 ; CHECK-SAME: "source variables":6
 ; CHECK-SAME: "variables with location":6
 
 ;header.h:
 ;extern "C" int getchar();
 ;template<typename T> T __attribute__((always_inline)) inlined() {
 ;  if (getchar()=='a') {

llvm/trunk/test/tools/llvm-dwarfdump/X86/stats-inlining-single-cu.ll

 ; RUN: llc -O0 %s -o - -filetype=obj \
 ; RUN:   | llvm-dwarfdump -statistics - | FileCheck %s
 
 ; This test serves as a baseline / sanity-check for stats-inlining-multi-cu.ll
 ; The results for both tests should be identical.
 
 ; CHECK:      "source functions":4,
-; CHECK-SAME: "inlined functions":5,
+; CHECK-SAME: "inlined functions":2,
 ; CHECK-SAME: "unique source variables":4
 ; CHECK-SAME: "source variables":6
 ; CHECK-SAME: "variables with location":6
 
 ;header.h:
 ;extern "C" int getchar();
 ;template<typename T> T __attribute__((always_inline)) inlined() {
 ;  if (getchar()=='a') {

llvm/trunk/tools/llvm-dwarfdump/Statistics.cpp

struct PerFunctionStats {
   /// Number of variables with location across all inlined instances.
   unsigned TotalVarWithLoc = 0;
   /// Number of constants with location across all inlined instances.
   unsigned ConstantMembers = 0;
   /// List of all Variables in this function.
   StringSet<> VarsInFunction;
   /// Compile units also cover a PC range, but have this flag set to false.
   bool IsFunction = false;
+  /// Verify function definition has PC addresses (for detecting when
+  /// a function has been inlined everywhere).
+  bool HasPCAddresses = false;
 };
 
 /// Holds accumulated global statistics about DIEs.
 struct GlobalStats {
   /// Total number of PC range bytes covered by DW_AT_locations.
   unsigned ScopeBytesCovered = 0;
   /// Total number of PC range bytes in each variable's enclosing scope,
   /// starting from the first definition of the variable.
if (BytesInScope) {
     // Adjust for the fact the variables often start their lifetime in the
     // middle of the scope.
     BytesInScope -= OffsetToFirstDefinition;
     // Turns out we have a lot of ranges that extend past the lexical scope.
     GlobalStats.ScopeBytesCovered += std::min(BytesInScope, BytesCovered);
     GlobalStats.ScopeBytesFromFirstDefinition += BytesInScope;
     assert(GlobalStats.ScopeBytesCovered <=
            GlobalStats.ScopeBytesFromFirstDefinition);
-  } else {
+  } else if (Die.getTag() == dwarf::DW_TAG_member) {
     FnStats.ConstantMembers++;
+  } else {
+    FnStats.TotalVarWithLoc += (unsigned)HasLoc;
   }
 }
 
 /// Recursively collect debug info quality metrics.
 static void collectStatsRecursive(DWARFDie Die, std::string FnPrefix,
                                   std::string VarPrefix, uint64_t ScopeLowPC,
                                   uint64_t BytesInScope,
                                   uint32_t InlineDepth,
if (IsFunction || IsInlinedFunction || IsBlock) {
     if (Die.getTag() == dwarf::DW_TAG_subprogram ||
         Die.getTag() == dwarf::DW_TAG_inlined_subroutine)
       VarPrefix = "v";
 
     // Ignore forward declarations.
     if (Die.find(dwarf::DW_AT_declaration))
       return;
 
+    // PC Ranges.
+    auto RangesOrError = Die.getAddressRanges();
+    if (!RangesOrError) {
+      llvm::consumeError(RangesOrError.takeError());
+      return;
+    }
+
+    auto Ranges = RangesOrError.get();
+    uint64_t BytesInThisScope = 0;
+    for (auto Range : Ranges)
+      BytesInThisScope += Range.HighPC - Range.LowPC;
+    ScopeLowPC = getLowPC(Die);
+
     // Count the function.
     if (!IsBlock) {
       StringRef Name = Die.getName(DINameKind::LinkageName);
       if (Name.empty())
         Name = Die.getName(DINameKind::ShortName);
       FnPrefix = Name;
       // Skip over abstract origins.
       if (Die.find(dwarf::DW_AT_inline))
         return;
       // We've seen an (inlined) instance of this function.
       auto &FnStats = FnStatMap[Name];
+      if (IsInlinedFunction)
         FnStats.NumFnInlined++;
       FnStats.IsFunction = true;
+      if (BytesInThisScope && !IsInlinedFunction)
+        FnStats.HasPCAddresses = true;
     }
 
-    // PC Ranges.
-    auto RangesOrError = Die.getAddressRanges();
-    if (!RangesOrError) {
-      llvm::consumeError(RangesOrError.takeError());
-      return;
-    }
-
-    auto Ranges = RangesOrError.get();
-    uint64_t BytesInThisScope = 0;
-    for (auto Range : Ranges)
-      BytesInThisScope += Range.HighPC - Range.LowPC;
-    ScopeLowPC = getLowPC(Die);
-
     if (BytesInThisScope) {
       BytesInScope = BytesInThisScope;
       if (IsFunction)
         GlobalStats.FunctionSize += BytesInThisScope;
       else if (IsInlinedFunction && InlineDepth == 0)
         GlobalStats.InlineFunctionSize += BytesInThisScope;
     }
   } else {
bool collectStatsForObjectFile(ObjectFile &Obj, DWARFContext &DICtx,
   StringMap<PerFunctionStats> Statistics;
   for (const auto &CU : static_cast<DWARFContext *>(&DICtx)->compile_units())
     if (DWARFDie CUDie = CU->getUnitDIE(false))
       collectStatsRecursive(CUDie, "/", "g", 0, 0, 0, Statistics, GlobalStats);
 
   /// The version number should be increased every time the algorithm is changed
   /// (including bug fixes). New metrics may be added without increasing the
   /// version.
-  unsigned Version = 1;
+  unsigned Version = 2;
   unsigned VarTotal = 0;
   unsigned VarUnique = 0;
   unsigned VarWithLoc = 0;
   unsigned NumFunctions = 0;
   unsigned NumInlinedFunctions = 0;
   for (auto &Entry : Statistics) {
     PerFunctionStats &Stats = Entry.getValue();
     unsigned TotalVars = Stats.VarsInFunction.size() * Stats.NumFnInlined;
+    // Count variables in concrete out-of-line functions and in global scope.
+    if (Stats.HasPCAddresses || !Stats.IsFunction)
+      TotalVars += Stats.VarsInFunction.size();
     unsigned Constants = Stats.ConstantMembers;
     VarWithLoc += Stats.TotalVarWithLoc + Constants;
-    VarTotal += TotalVars + Constants;
+    VarTotal += TotalVars;
     VarUnique += Stats.VarsInFunction.size();
     LLVM_DEBUG(for (auto &V : Stats.VarsInFunction) llvm::dbgs()
                << Entry.getKey() << ": " << V.getKey() << "\n");
     NumFunctions += Stats.IsFunction;
     NumInlinedFunctions += Stats.IsFunction * Stats.NumFnInlined;
   }
 
   // Print summary.