[PATCH] llvm-cov: Added -a option for block data.

[Yuchen Wu]

Updated tests.

----------------------------------------
From: yuchenericwu at hotmail.com
To: mail at justinbogner.com; llvm-commits at cs.uiuc.edu
Subject: RE: [PATCH] llvm-cov: Added -a option for block data.
Date: Mon, 2 Dec 2013 23:11:14 -0500


Here is the new patch, as per your suggestions. Comments below:

----------------------------------------
> From: mail at justinbogner.com
> To: llvm-commits at cs.uiuc.edu
> CC: llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH] llvm-cov: Added -a option for block data.
> Date: Mon, 2 Dec 2013 15:12:41 -0800
>
> Yuchen Wu <yuchenericwu at hotmail.com> writes:
>> Minor update: Removed isTrivialBlock(). It is no longer necessary due
>> to r195513.
> ...
>> From 69e6db89483b170dc34eea0adbdaf28bde3b10e5 Mon Sep 17 00:00:00 2001
>> From: Yuchen Wu <yuchen_wu at apple.com>
>> Date: Wed, 20 Nov 2013 13:21:29 -0800
>> Subject: [PATCH 06/10] llvm-cov: Added -a option for block data.
>>
>> Similar to gcov, llvm-cov will now print out the block count at the end
>> of each block. Multiple blocks can end on the same line.
>>
>> One computational difference is by using -a, llvm-cov will no longer
>> simply add the block counts together to form a line count. Instead, it
>> will take the maximum of the block counts on that line. This has a
>> similar effect to what gcov does, but generates more correct counts in
>> certain scenarios.
>> ---
>> include/llvm/Support/GCOV.h | 12 ++++++++++-
>> lib/IR/GCOV.cpp | 49 +++++++++++++++++++++++++++++++++++----------
>> tools/llvm-cov/llvm-cov.cpp | 6 ++++--
>> 3 files changed, 53 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/llvm/Support/GCOV.h b/include/llvm/Support/GCOV.h
>> index 3298132..c12b054 100644
>> --- a/include/llvm/Support/GCOV.h
>> +++ b/include/llvm/Support/GCOV.h
>> @@ -37,6 +37,13 @@ namespace GCOV {
>> };
>> } // end GCOV namespace
>>
>> +/// GCOVOptions - A struct for passing gcov options between functions.
>> +struct GCOVOptions {
>> + GCOVOptions(bool A): AllBlocks(A) {}
>
> Is 'A' really better than 'AllBlocks'?

As the number of options grow, it looks cleaner and shorter to have the options' shorthand as the variable names.

>> +
>> + bool AllBlocks;
>> +};
>> +
>> /// GCOVBuffer - A wrapper around MemoryBuffer to provide GCOV specific
>> /// read operations.
>> class GCOVBuffer {
>> @@ -222,6 +229,7 @@ public:
>> bool readGCNO(GCOVBuffer &Buffer, GCOV::GCOVFormat Format);
>> bool readGCDA(GCOVBuffer &Buffer, GCOV::GCOVFormat Format);
>> StringRef getFilename() const { return Filename; }
>> + size_t getNumBlocks() const { return Blocks.size(); }
>> void dump() const;
>> void collectLineCounts(FileInfo &FI);
>> private:
>> @@ -252,6 +260,7 @@ public:
>> DstEdges.push_back(Edge);
>> }
>> void addLine(uint32_t N) { Lines.push_back(N); }
>> + uint32_t getLastLine() const { return Lines.back(); }
>> void addCount(size_t Index, uint64_t N);
>> uint64_t getCount() const { return Counter; }
>> size_t getNumSrcEdges() const { return SrcEdges.size(); }
>> @@ -282,7 +291,8 @@ public:
>> }
>> void setRunCount(uint32_t Runs) { RunCount = Runs; }
>> void setProgramCount(uint32_t Programs) { ProgramCount = Programs; }
>> - void print(StringRef gcnoFile, StringRef gcdaFile) const;
>> + void print(StringRef gcnoFile, StringRef gcdaFile,
>> + const GCOVOptions &Options) const;
>> private:
>> StringMap<LineData> LineInfo;
>> uint32_t RunCount;
>> diff --git a/lib/IR/GCOV.cpp b/lib/IR/GCOV.cpp
>> index b26d303..26a6e79 100644
>> --- a/lib/IR/GCOV.cpp
>> +++ b/lib/IR/GCOV.cpp
>> @@ -352,7 +352,8 @@ void GCOVBlock::dump() const {
>> // FileInfo implementation.
>>
>> /// print - Print source files with collected line count information.
>> -void FileInfo::print(StringRef gcnoFile, StringRef gcdaFile) const {
>> +void FileInfo::print(StringRef gcnoFile, StringRef gcdaFile,
>> + const GCOVOptions &Options) const {
>> for (StringMap<LineData>::const_iterator I = LineInfo.begin(),
>> E = LineInfo.end(); I != E; ++I) {
>> StringRef Filename = I->first();
>> @@ -375,16 +376,26 @@ void FileInfo::print(StringRef gcnoFile, StringRef gcdaFile) const {
>> OS << " -: 0:Runs:" << RunCount << "\n";
>> OS << " -: 0:Programs:" << ProgramCount << "\n";
>>
>> - const LineData &L = I->second;
>> - uint32_t i = 0;
>> - while (!AllLines.empty()) {
>> - LineData::const_iterator BlocksIt = L.find(i);
>> - if (BlocksIt != L.end()) {
>> + const LineData &Line = I->second;
>> + for (uint32_t i = 0; !AllLines.empty(); ++i) {
>> + LineData::const_iterator BlocksIt = Line.find(i);
>> +
>> + // Add up the block counts to form line counts.
>> + if (BlocksIt != Line.end()) {
>
> These are some nice cleanups, but please break them out into their own
> patch.

Okay, done. r196194.

>> const BlockVector &Blocks = BlocksIt->second;
>> uint64_t LineCount = 0;
>> for (BlockVector::const_iterator I = Blocks.begin(), E = Blocks.end();
>> I != E; ++I) {
>> - LineCount += (*I)->getCount();
>> + const GCOVBlock *Block = *I;
>> + // If -a option is enabled, only take the highest block count for
>> + // that line.
>> + if (Options.AllBlocks) {
>> + uint64_t BlockCount = Block->getCount();
>> + LineCount = LineCount> BlockCount ? LineCount : BlockCount;
>> + }
>> + // Otherwise, sum up all of the block counts.
>> + else
>> + LineCount += Block->getCount();
>
> It's more readable to put these comments inside the if/else scopes, and
> "If -a option is enabled" is a bit redundant with "if (Options.AllBlocks)"

Fair point.

>> }
>> if (LineCount == 0)
>> OS << " #####:";
>> @@ -394,11 +405,27 @@ void FileInfo::print(StringRef gcnoFile, StringRef gcdaFile) const {
>> OS << " -:";
>> }
>> std::pair<StringRef, StringRef> P = AllLines.split('\n');
>> - if (AllLines != P.first)
>> - OS << format("%5u:", i+1) << P.first;
>> - OS << "\n";
>> + OS << format("%5u:", i+1) << P.first << "\n";
>
> This looks like a behaviour change, but I don't see how it's related.

Also committed in r196194.

>> AllLines = P.second;
>> - ++i;
>> +
>> + // If -a option is enabled, output the counts for each block at the last
>> + // line of the block.
>> + if (Options.AllBlocks && BlocksIt != Line.end()) {
>> + uint32_t BlockNo = 0;
>> + const BlockVector &Blocks = BlocksIt->second;
>> + for (BlockVector::const_iterator I = Blocks.begin(), E = Blocks.end();
>> + I != E; ++I) {
>
> The whitespace is a bit funny here. The I should line up under the B in
> BlockVector.

Actually the style of this file (as I found it) seems to prefer adding an extra 2 whitespaces to multiline for statements.

_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits 		 	   		  
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-llvm-cov-Added-a-option-for-block-data.patch
Type: application/octet-stream
Size: 12006 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131209/b7b796ce/attachment.obj>