This is an archive of the discontinued LLVM Phabricator instance.

[Coverage] Support for making test data for more than one function.
ClosedPublic

Authored by ikudrin on Apr 4 2016, 8:24 AM.

Diff Detail

Event Timeline

ikudrin updated this revision to Diff 52560.Apr 4 2016, 8:24 AM
ikudrin retitled this revision from to [Coverage] Support for making test data for more than one function..
ikudrin updated this object.
ikudrin added reviewers: bogner, davidxl, vsk.
ikudrin added a subscriber: llvm-commits.
vsk edited edge metadata.Apr 6 2016, 10:40 AM

Could you explain the difference between global file indices and an index in a function?

llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp
86

Please add a comment explaining the what the map contains. Also, would an llvm::SmallDenseMap work here?

In D18758#393415, @vsk wrote:

Could you explain the difference between global file indices and an index in a function?

This is intended to be closer to the serialization format of coverage mapping. All functions share the same array of file names, but each function has its own set of files and maps its local indices to global ones. See RawCoverageMappingReader::read().

davidxl added inline comments.Apr 9 2016, 3:02 PM
llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp
56

Is this field needed? It can be a local variable.

174

Split this function into two methods:

  1. writeAndReadCoverageRegions
  2. loadCoverageMapping

loadCoverageMapping calls 1) followed by call to CoverageMapping::load

Calls to readProfCount can also be folded into 2).

Function 1) can be called in many other places as well.

ikudrin updated this revision to Diff 53720.Apr 14 2016, 8:03 AM
ikudrin updated this object.
ikudrin edited edge metadata.
  • Rebased on the top commit.
  • Made Expressions local.
  • Added a comment for ReverseVirtualFileMapping and changed its type to llvm::SmallDenseMap.
  • Extracted writeAndReadCoverageRegions from loadCoverageMapping.
  • Added a call to readProfCount into loadCoverageMapping.
davidxl edited edge metadata.Apr 14 2016, 10:16 AM

nice cleanup!

llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp
86

an --> the

88

Is this mapping strictly needed? Can global file index be used without creating function level file index?

davidxl accepted this revision.Apr 14 2016, 2:47 PM
davidxl edited edge metadata.

lgtm.

llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp
88

It is needed. The virtual FileId for each function record is the index to the array of region sub-arrays in the coverage mapping.

Perhaps you can add more comments to clarify.

118

Add a comment to this method. Something in the line of ..

Returns the file index of file 'Name' for the current function. It also computes the global file index
for the file and maps the global index to the local index that is returned (ReverseVirtualFileMapping).
The ReverseVirtualFileMapping is used to create the virtual file mapping (from local fileId to global Index)
in the head of the per-function coverage mapping data.

This revision is now accepted and ready to land.Apr 14 2016, 2:47 PM

Thanks a lot for the comments!

This revision was automatically updated to reflect the committed changes.