Page MenuHomePhabricator

[llvm-dwarfdump][Statistics] Distinguish functions/variables with same name across different CUs
ClosedPublic

Authored by krisb on Jan 15 2020, 11:57 AM.

Details

Summary
Different variables and functions might have the same name in different CU.
To calculate 'Availability' metric more accurate (i.e. to avoid getting
availability above 100%), we need to have some additional logic to
distinguish between them.

The patch introduces a DIE identifier that consists of a function/variable name
and declaration information: a filename and a line number. This allows
distinguishing different functions/variables (different means declared in
different files/lines) with the same name, keeping duplicates counted
as duplicates.

This patch is the first one from a set of 6 that aims to improve 'Availability' statistics. 

For testing purposes, I used clang-3.4 non-optimized build and here are
the results for 10 most 'heavy' binaries before and after all the 6 patches:
             |   "source   |      baseline(master)    |         patched          |
Binary name  |  variables" | "Availability" | time(s) | "Availability" | time(s) |
----------------------------------------------------------------------------------
opt          |    637923   |         214%   |   1.644 |         100%   |   2.47  |
lli          |    362669   |         177%   |   0.898 |          99%   |   1.346 |
diagtool     |    459671   |         184%   |   1.399 |         100%   |   2.103 |
arcmt-test   |    572726   |         180%   |   1.665 |         100%   |   2.53  |
llc          |    527603   |         200%   |   1.431 |         100%   |   2.282 |
clang-3.4    |   1445548   |         208%   |   4.33  |         100%   |   6.413 |
llvm-lto     |    629415   |         208%   |   1.652 |         100%   |   2.666 |
clang-check  |    621696   |         195%   |   2.108 |         100%   |   3.302 |
clang-format |    470768   |         184%   |   1.429 |          99%   |   2.154 |
llvm-c-test  |    478700   |         200%   |   1.302 |         100%   |   2.033 |

Diff Detail

Event Timeline

krisb created this revision.Jan 15 2020, 11:57 AM
Herald added a project: Restricted Project. · View Herald Transcript

This five patches reduces 'total availability' metric of clang binary (debug build) from 126% to 87%.

For a debug build you'd expect 100% where are the missing 13% coming from?

Otherwise this change seems to go into the right direction.

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

When are these generated? Could we write something more descriptive into the comment?

Nice, thanks! Please post the rest of the patches, when you are ready, and make a stack of the patches.

This five patches reduces 'total availability' metric of clang binary
(debug build) from 126% to 87%.

I guess we are talking about the build with -g -O2?

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

Could you please add an explanation here why '(probably)'?

Nice, thanks! Please post the rest of the patches, when you are ready, and make a stack of the patches.

This five patches reduces 'total availability' metric of clang binary
(debug build) from 126% to 87%.

I guess we are talking about the build with -g -O2?

Or any level of optimization higher than -O0 + -g.

krisb updated this revision to Diff 238955.Jan 18 2020, 10:07 AM
krisb marked 3 inline comments as done.

Applied the comments.

krisb edited the summary of this revision. (Show Details)Jan 18 2020, 10:08 AM

@aprantl @djtodoro thanks for the comments!

I updated the description adding more clear and reproducible testing results for 'O0' build.
There is no 100% of availability even on non-optimized build because of 2 issues:

  • some variables really don't have locations. They are usually artificial or global variables, but the number of them is really small (for example, 46/1435958 for clang-34 binary).
  • parameters may have different DW_AT_decl_file and DW_AT_decl_line among instances of the same function. It looks like this happens when a function has its declaration and definition in different header files. So, one can find something like this in dwarf (these are two concrete out-of-line instances of the same 'foo' function).
DW_TAG_subprogram
  DW_AT_name  ("foo")
  DW_AT_decl_file ("definition.h")
  DW_AT_decl_line (222)
  DW_TAG_formal_parameter
    DW_AT_name  ("__a")
    DW_AT_decl_file ("declaration.h")
    DW_AT_decl_line (370)
  DW_TAG_formal_parameter
    DW_AT_name  ("__b")
    DW_AT_decl_file ("declaration.h")
    DW_AT_decl_line (370)

DW_TAG_subprogram
  DW_AT_name  ("foo")
  DW_AT_decl_file ("definition.h")
  DW_AT_decl_line (222)
  DW_TAG_formal_parameter
    DW_AT_name  ("__a")
    DW_AT_decl_file ("definition.h")
    DW_AT_decl_line (222)
  DW_TAG_formal_parameter
    DW_AT_name  ("__b")
    DW_AT_decl_file ("definition.h")
    DW_AT_decl_line (222)

So, this increases number of 'total vars' because these 'a' and 'b' variables counted twice.

I also was a bit optimistic about how this patch(es) affects the performance of the statistics, it actually became two times slower than before the patches.

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

I haven't had a chance to debug such cases properly, but I believe they are a result of some bugs in dwarf generation flow.
Such functions have no attributes at all but can contain some children (usually structures or classes). So, I removed the check by now cause it's better to fix the issue in dwarf generation than workaround it in statistics.

Thanks for doing this!

So, this increases number of 'total vars' because these 'a' and 'b' variables counted twice.

I see... Can we do something to avoid the additional calculation? Somehow to recognize we already observed the a and b, although they have different DIE ID.

I also was a bit optimistic about how this patch(es) affects the performance of the statistics, it actually became two times slower than before the patches.

What patch from the stack adds the overhead? This one?

krisb added a comment.Jan 20 2020, 8:09 AM

So, this increases number of 'total vars' because these 'a' and 'b' variables counted twice.

I see... Can we do something to avoid the additional calculation? Somehow to recognize we already observed the a and b, although they have different DIE ID.

I also was a bit optimistic about how this patch(es) affects the performance of the statistics, it actually became two times slower than before the patches.

What patch from the stack adds the overhead? This one?

Yeah, this one mostly causes the overhead.
Actually, adding declaration info for ID is only necessary for functions, global variables, and members (because we need to handle static/inline functions/variables, and don't respect namespaces, classes, and structures). For local variables and parameters, Prefix + Name should be enough to distinguish all the cases (because their prefix already guaranteed uniqueness, for parameters the prefix is going to be added in D73003).
This approach shows better results:

             |   "source   |      baseline(master)    |         patched          |
Binary name  |  variables" | "Availability" | time(s) | "Availability" | time(s) |
----------------------------------------------------------------------------------
opt          |    637923   |         214%   |   1.644 |         100%   |   2.47  |
lli          |    362669   |         177%   |   0.898 |          99%   |   1.346 |
diagtool     |    459671   |         184%   |   1.399 |         100%   |   2.103 |
arcmt-test   |    572726   |         180%   |   1.665 |         100%   |   2.53  |
llc          |    527603   |         200%   |   1.431 |         100%   |   2.282 |
clang-3.4    |   1445548   |         208%   |   4.33  |         100%   |   6.413 |
llvm-lto     |    629415   |         208%   |   1.652 |         100%   |   2.666 |
clang-check  |    621696   |         195%   |   2.108 |         100%   |   3.302 |
clang-format |    470768   |         184%   |   1.429 |          99%   |   2.154 |
llvm-c-test  |    478700   |         200%   |   1.302 |         100%   |   2.033 |

It also reduces the overhead of this patch to up to 61% (instead of up to 100% previously).
With this approach, we have a really small amount of 'collisions' but still have some.
I'll update the patch when I find the reason for the 'collisions' left.

krisb updated this revision to Diff 239602.Jan 22 2020, 8:06 AM
krisb edited the summary of this revision. (Show Details)

Use declaration info only to identify functions, global variables, and members.

krisb added a comment.Jan 22 2020, 8:21 AM

@djtodoro those two binaries with 99% of availability just have some variables w/o locations (mostly artificial 'this' parameters of member functions).
But this change reveals one more interesting case.
For code like this:

int foo() {
	if (cond1) {
		static const int a = 1;
		...
	}
	if (cond2) {
		static const int a = 1;
		...
	}
}

clang (unlike gcc) generates both a variables in the function scope, not in a corresponded DW_TAG_lexical_block (as gcc does).
I'm not sure clang is right here, cause a - despite it is static - isn't visible outside the if.
What do you think about this?

Hmm.. it looks lika a bug.

@aprantl What do you think?

aprantl accepted this revision.Fri, Jan 24, 2:25 PM

Hmm.. it looks lika a bug.

@aprantl What do you think?

If they are two separate (storage) variables that is a bug. Can you file a PR on bugzilla (libraries/debuginfo) and CC the usual suspects?

This revision is now accepted and ready to land.Fri, Jan 24, 2:25 PM
krisb added a comment.Tue, Jan 28, 6:25 AM

@djtodoro @aprantl thanks for reviewing this whole set of patches!

I've opened a PR for that issue with static variables https://bugs.llvm.org/show_bug.cgi?id=44695

This revision was automatically updated to reflect the committed changes.