Page MenuHomePhabricator

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

Authored by cmtice on Wed, Feb 6, 2:40 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

cmtice created this revision.Wed, Feb 6, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Feb 6, 2:40 PM
aprantl added inline comments.Wed, Feb 6, 2:53 PM
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 updated this revision to Diff 185788.Thu, Feb 7, 9:08 AM

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 updated this revision to Diff 185806.Thu, Feb 7, 10:06 AM

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".

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 added inline comments.Thu, Feb 7, 10:56 AM
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 updated this revision to Diff 185824.Thu, Feb 7, 11:02 AM

This increases the version number, as requested.

aprantl added inline comments.Thu, Feb 7, 1:14 PM
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 added inline comments.Thu, Feb 7, 1:34 PM
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 updated this revision to Diff 185885.Thu, Feb 7, 4:10 PM

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 added inline comments.Thu, Feb 7, 4:26 PM
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 added a comment.Thu, Feb 7, 4:33 PM

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 accepted this revision.Thu, Feb 7, 4:38 PM

Ah, good catch. Makes sense!

thanks!

This revision is now accepted and ready to land.Thu, Feb 7, 4:38 PM
This revision was automatically updated to reflect the committed changes.