[llvm-dwarfdump][Statistics] Don't count coverage less than 1% as 0%

[krisb]

Summary

Currently, variables with debug info coverage between 0% and 1% are put into
zero-bucket. D70548 changed the way statistics calculate a variable's coverage:
we began to use enclosing scope rather than a possible variable life range.
Thus more variables might be moved to zero-bucket despite they have some debug
info coverage.
The patch is to distinguish between a variable that has location info but
it's significantly less than its enclosing scope and a variable that doesn't
have it at all.

[aprantl]

That sounds reasonable to me.

[djtodoro]

LGTM as well, thanks!

Please update the llvm/docs/CommandGuide/llvm-locstats.rst with the newest output.

[djtodoro]

Hm, the bucket name ‘>0-9%' does not look nice to me as well.

[krisb]

@djtodoro , yeah, it doesn't look good.
I also considered an option to rename the first bucket to smth like "No location info" or "Not covered" or something similar then the second can be safely named '0-9%'.
Or simply to use '0-9%' together with '0%' which might be a bit unclear, but as docs say 'The line 0% shows the number and the percentage of DIEs with no location information', it should not be confusing.

[dblaikie]

In D71070#1774223, @krisb wrote:

@djtodoro , yeah, it doesn't look good.
I also considered an option to rename the first bucket to smth like "No location info" or "Not covered" or something similar then the second can be safely named '0-9%'.
Or simply to use '0-9%' together with '0%' which might be a bit unclear, but as docs say 'The line 0% shows the number and the percentage of DIEs with no location information', it should not be confusing.

does this bucket include 9%? I guess the next bucket starts at 10%, so 0-9 includes 9.99%? so technically this one is (0, 10)% and the next one is [10, 91)% etc... (or I guess you could put the % inside the braces/parens)? but that might be insufficiently obvious to read?

[djtodoro]

In D71070#1774443, @dblaikie wrote:

In D71070#1774223, @krisb wrote:

@djtodoro , yeah, it doesn't look good.
I also considered an option to rename the first bucket to smth like "No location info" or "Not covered" or something similar then the second can be safely named '0-9%'.
Or simply to use '0-9%' together with '0%' which might be a bit unclear, but as docs say 'The line 0% shows the number and the percentage of DIEs with no location information', it should not be confusing.

does this bucket include 9%? I guess the next bucket starts at 10%, so 0-9 includes 9.99%? so technically this one is (0, 10)% and the next one is [10, 91)% etc... (or I guess you could put the % inside the braces/parens)? but that might be insufficiently obvious to read?

I am also thinking about combination of the [/] and (/). That will clarify things here.

[krisb]

@dblaikie , @djtodoro sorry for paying so much attention to details here, but does this one work with you?

     cov%          samples       percentage(~)
-------------------------------------------------
   0%                    1              16%
   ( 0, 10%)             0               0%
   [10, 20%)             0               0%
   [20, 30%)             0               0%
   [30, 40%)             0               0%
   [40, 50%)             0               0%
   [50, 60%)             1              16%
   [60, 70%)             0               0%
   [70, 80%)             0               0%
   [80, 90%)             1              16%
   [90,100%)             0               0%
   100%                  3              50%

And just mention here how the previously suggested option looks like:

      cov%          samples       percentage(~)
-------------------------------------------------
   No location info      1              16%
   0-9%                  0               0%
   10-19%                0               0%
   20-29%                0               0%
   30-39%                0               0%
   40-49%                0               0%
   50-59%                1              16%
   60-69%                0               0%
   70-79%                0               0%
   80-89%                1              16%
   90-99%                0               0%
   100%                  3              50%

[djtodoro]

@krisb Thanks for this again. First option seems better to me.

(Just please make sure, when we decide the final look, the docs gets updated accordingly.)

[dblaikie]

either looks OK to me - between those two, yeah, the ()[)/etc (the first) version is probably better. But I'll leave it to other folks more invested in statistics to make the final call.

[krisb]

@djtodoro , @dblaikie thanks for your opinions! I created a separate review for this change https://reviews.llvm.org/D71366. Please, take a look if you have a chance.

[djtodoro]

This one should be rebased on top of the D71366.

[krisb]

Updated the doc, rebased on D71366.

[spatel]

I'm seeing test failures on trunk after this commit:

FAIL: LLVM :: tools/llvm-locstats/locstats.ll (33082 of 34788)
******************** TEST 'LLVM :: tools/llvm-locstats/locstats.ll' FAILED ********************
Script:
--
: 'RUN: at line 3';   /Users/spatel/GitHub/llvm-project/release/bin/llc /Users/spatel/GitHub/llvm-project/llvm/test/tools/llvm-locstats/locstats.ll -o /Users/spatel/GitHub/llvm-project/release/test/tools/llvm-locstats/Output/locstats.ll.tmp0.o -filetype=obj
: 'RUN: at line 4';   '/usr/bin/python' /Users/spatel/GitHub/llvm-project/release/./bin/llvm-locstats /Users/spatel/GitHub/llvm-project/release/test/tools/llvm-locstats/Output/locstats.ll.tmp0.o | /Users/spatel/GitHub/llvm-project/release/bin/FileCheck /Users/spatel/GitHub/llvm-project/llvm/test/tools/llvm-locstats/locstats.ll --check-prefix=LOCSTATS
--
Exit Code: 2

Command Output (stderr):
--
Traceback (most recent call last):
  File "/Users/spatel/GitHub/llvm-project/release/./bin/llvm-locstats", line 212, in <module>
    Main()
  File "/Users/spatel/GitHub/llvm-project/release/./bin/llvm-locstats", line 190, in Main
    variables_coverage_map[cov_bucket] = json_parsed[cov_category]
KeyError: 'variables with [1%,10%) of its scope covered'
FileCheck error: '-' is empty.
FileCheck command line:  /Users/spatel/GitHub/llvm-project/release/bin/FileCheck /Users/spatel/GitHub/llvm-project/llvm/test/tools/llvm-locstats/locstats.ll --check-prefix=LOCSTATS

[krisb]

@spatel thanks for reporting this!
Could you, please, check that llvm-locstat wasn't cached in the build/install directory? llvm-locstat should look for a variables with (0%,10%) of its scope covered as was changed by the patch, so I guess it didn't get updated for some reason.

[spatel]

In D71070#1783599, @krisb wrote:

@spatel thanks for reporting this!
Could you, please, check that llvm-locstat wasn't cached in the build/install directory? llvm-locstat should look for a variables with (0%,10%) of its scope covered as was changed by the patch, so I guess it didn't get updated for some reason.

That might've been it. I just rebuilt everything, and no more fails. Sorry for the noise.

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

 ; RUN: llc -debug-entry-values %s -o - -filetype=obj \
 ; RUN:   | llvm-dwarfdump -statistics - | FileCheck %s
 ;
 ; CHECK: "entry value scope bytes covered":5
 ; CHECK: "formal params scope bytes total":20
 ; CHECK: "formal params scope bytes covered":20
 ; CHECK: "formal params entry value scope bytes covered":5
 ; CHECK: "vars scope bytes total":90
 ; CHECK: "vars scope bytes covered":60
 ; CHECK: "vars entry value scope bytes covered":0
 ; CHECK: "total variables procesed by location statistics":6
 ; CHECK: "variables with 0% of its scope covered":1
-; CHECK: "variables with 1-9% of its scope covered":0
+; CHECK: "variables with >0-9% of its scope covered":0
 ; CHECK: "variables with 10-19% of its scope covered":0
 ; CHECK: "variables with 20-29% of its scope covered":0
 ; CHECK: "variables with 30-39% of its scope covered":0
 ; CHECK: "variables with 40-49% of its scope covered":0
 ; CHECK: "variables with 50-59% of its scope covered":1
 ; CHECK: "variables with 60-69% of its scope covered":0
 ; CHECK: "variables with 70-79% of its scope covered":0
 ; CHECK: "variables with 80-89% of its scope covered":1
 ; CHECK: "variables with 90-99% of its scope covered":0
 ; CHECK: "variables with 100% of its scope covered":3
 ; CHECK: "variables (excluding the debug entry values) with 0% of its scope covered":1
-; CHECK: "variables (excluding the debug entry values) with 1-9% of its scope covered":0
+; CHECK: "variables (excluding the debug entry values) with >0-9% of its scope covered":0
 ; CHECK: "variables (excluding the debug entry values) with 10-19% of its scope covered":0
 ; CHECK: "variables (excluding the debug entry values) with 20-29% of its scope covered":0
 ; CHECK: "variables (excluding the debug entry values) with 30-39% of its scope covered":0
 ; CHECK: "variables (excluding the debug entry values) with 40-49% of its scope covered":0
 ; CHECK: "variables (excluding the debug entry values) with 50-59% of its scope covered":2
 ; CHECK: "variables (excluding the debug entry values) with 60-69% of its scope covered":0
 ; CHECK: "variables (excluding the debug entry values) with 70-79% of its scope covered":0
 ; CHECK: "variables (excluding the debug entry values) with 80-89% of its scope covered":1
 ; CHECK: "variables (excluding the debug entry values) with 90-99% of its scope covered":0
 ; CHECK: "variables (excluding the debug entry values) with 100% of its scope covered":2
 ; CHECK: "total params procesed by location statistics":2
 ; CHECK: "params with 0% of its scope covered":0
-; CHECK: "params with 1-9% of its scope covered":0
+; CHECK: "params with >0-9% of its scope covered":0
 ; CHECK: "params with 10-19% of its scope covered":0
 ; CHECK: "params with 20-29% of its scope covered":0
 ; CHECK: "params with 30-39% of its scope covered":0
 ; CHECK: "params with 40-49% of its scope covered":0
 ; CHECK: "params with 50-59% of its scope covered":0
 ; CHECK: "params with 60-69% of its scope covered":0
 ; CHECK: "params with 70-79% of its scope covered":0
 ; CHECK: "params with 80-89% of its scope covered":0
 ; CHECK: "params with 90-99% of its scope covered":0
 ; CHECK: "params with 100% of its scope covered":2
 ; CHECK: "params (excluding the debug entry values) with 0% of its scope covered":0
-; CHECK: "params (excluding the debug entry values) with 1-9% of its scope covered":0
+; CHECK: "params (excluding the debug entry values) with >0-9% of its scope covered":0
 ; CHECK: "params (excluding the debug entry values) with 10-19% of its scope covered":0
 ; CHECK: "params (excluding the debug entry values) with 20-29% of its scope covered":0
 ; CHECK: "params (excluding the debug entry values) with 30-39% of its scope covered":0
 ; CHECK: "params (excluding the debug entry values) with 40-49% of its scope covered":0
 ; CHECK: "params (excluding the debug entry values) with 50-59% of its scope covered":1
 ; CHECK: "params (excluding the debug entry values) with 60-69% of its scope covered":0
 ; CHECK: "params (excluding the debug entry values) with 70-79% of its scope covered":0
 ; CHECK: "params (excluding the debug entry values) with 80-89% of its scope covered":0
 ; CHECK: "params (excluding the debug entry values) with 90-99% of its scope covered":0
 ; CHECK: "params (excluding the debug entry values) with 100% of its scope covered":1
 ; CHECK: "total vars procesed by location statistics":4
 ; CHECK: "vars with 0% of its scope covered":1
-; CHECK: "vars with 1-9% of its scope covered":0
+; CHECK: "vars with >0-9% of its scope covered":0
 ; CHECK: "vars with 10-19% of its scope covered":0
 ; CHECK: "vars with 20-29% of its scope covered":0
 ; CHECK: "vars with 30-39% of its scope covered":0
 ; CHECK: "vars with 40-49% of its scope covered":0
 ; CHECK: "vars with 50-59% of its scope covered":1
 ; CHECK: "vars with 60-69% of its scope covered":0
 ; CHECK: "vars with 70-79% of its scope covered":0
 ; CHECK: "vars with 80-89% of its scope covered":1
 ; CHECK: "vars with 90-99% of its scope covered":0
 ; CHECK: "vars with 100% of its scope covered":1
 ; CHECK: "vars (excluding the debug entry values) with 0% of its scope covered":1
-; CHECK: "vars (excluding the debug entry values) with 1-9% of its scope covered":0
+; CHECK: "vars (excluding the debug entry values) with >0-9% of its scope covered":0
 ; CHECK: "vars (excluding the debug entry values) with 10-19% of its scope covered":0
 ; CHECK: "vars (excluding the debug entry values) with 20-29% of its scope covered":0
 ; CHECK: "vars (excluding the debug entry values) with 30-39% of its scope covered":0
 ; CHECK: "vars (excluding the debug entry values) with 40-49% of its scope covered":0
 ; CHECK: "vars (excluding the debug entry values) with 50-59% of its scope covered":1
 ; CHECK: "vars (excluding the debug entry values) with 60-69% of its scope covered":0
 ; CHECK: "vars (excluding the debug entry values) with 70-79% of its scope covered":0
 ; CHECK: "vars (excluding the debug entry values) with 80-89% of its scope covered":1

llvm/test/tools/llvm-locstats/locstats.ll

 ; UNSUPPORTED: system-windows
 ; REQUIRES: x86-registered-target
 ; RUN: llc %s -o %t0.o -filetype=obj
 ; RUN: %llvm-locstats %t0.o | FileCheck %s --check-prefix=LOCSTATS
 ;
 ; Test the llvm-locstats output.
 ; LOCSTATS: 0% 0 0%
-; LOCSTATS: 1-9% 0 0%
+; LOCSTATS: >0-9% 0 0%
 ; LOCSTATS: 10-19% 0 0%
 ; LOCSTATS: 20-29% 1 11%
 ; LOCSTATS: 30-39% 0 0%
 ; LOCSTATS: 40-49% 1 11%
 ; LOCSTATS: 50-59% 1 11%
 ; LOCSTATS: 60-69% 1 11%
 ; LOCSTATS: 70-79% 0 0%
 ; LOCSTATS: 80-89% 2 22%

llvm/tools/llvm-dwarfdump/Statistics.cpp

 
 /// Collect debug location statistics for one DIE.
 static void collectLocStats(uint64_t BytesCovered, uint64_t BytesInScope,
                             std::vector<unsigned> &VarParamLocStats,
                             std::vector<unsigned> &ParamLocStats,
                             std::vector<unsigned> &VarLocStats, bool IsParam,
                             bool IsLocalVar) {
   auto getCoverageBucket = [BytesCovered, BytesInScope]() -> unsigned {
-    unsigned LocBucket = 100 * (double)BytesCovered / BytesInScope;
-    if (LocBucket == 0) {
-      // No debug location at all for the variable.
+    if (BytesCovered == 0)
       return 0;
-    } else if (LocBucket == 100 || BytesCovered > BytesInScope) {
-      // Fully covered variable within its scope.
+    if (BytesCovered >= BytesInScope)
       return NumOfCoverageCategories - 1;
-    } else {
-      // Get covered range (e.g. 20%-29%).
-      LocBucket /= 10;
-      return LocBucket + 1;
-    }
+    unsigned LocBucket = 100 * (double)BytesCovered / BytesInScope;
   };
 
   unsigned CoverageBucket = getCoverageBucket();
   VarParamLocStats[CoverageBucket]++;
   if (IsParam)
     ParamLocStats[CoverageBucket]++;
   else if (IsLocalVar)
     VarLocStats[CoverageBucket]++;

 }
 static void printLocationStats(raw_ostream &OS,
                                const char *Key,
                                std::vector<unsigned> &LocationStats) {
   OS << ",\"" << Key << " with 0% of its scope covered\":"
      << LocationStats[0];
   LLVM_DEBUG(llvm::dbgs() << Key << " with 0% of its scope covered: "
                           << LocationStats[0] << '\n');
-  OS << ",\"" << Key << " with 1-9% of its scope covered\":"
+  OS << ",\"" << Key << " with >0-9% of its scope covered\":"
      << LocationStats[1];
-  LLVM_DEBUG(llvm::dbgs() << Key << " with 1-9% of its scope covered: "
+  LLVM_DEBUG(llvm::dbgs() << Key << " with >0-9% of its scope covered: "
                           << LocationStats[1] << '\n');
   for (unsigned i = 2; i < NumOfCoverageCategories - 1; ++i) {
     OS << ",\"" << Key << " with " << (i - 1) * 10 << "-" << i * 10 - 1
        << "% of its scope covered\":" << LocationStats[i];
     LLVM_DEBUG(llvm::dbgs()
                << Key << " with " << (i - 1) * 10 << "-" << i * 10 - 1
                << "% of its scope covered: " << LocationStats[i]);
   }

llvm/utils/llvm-locstats/llvm-locstats.py

 import os
 import sys
 from json import loads
 from math import ceil
 from subprocess import Popen, PIPE
 
 def coverage_buckets():
   yield '0%'
-  yield '1-9%'
+  yield '>0-9%'
   for start in range(10, 91, 10):
     yield '{0}-{1}%'.format(start, start + 9)
   yield '100%'
 
 def locstats_output(
   variables_total,
   variables_total_locstats,
   variables_with_loc,