[llvm] r228934 - CoverageMapping: Bitvectorize code. No functionality change.

[Nick Lewycky]

Justin Bogner wrote:
> Benjamin Kramer<benny.kra at googlemail.com>  writes:
>> Author: d0k
>> Date: Thu Feb 12 10:18:07 2015
>> New Revision: 228934
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=228934&view=rev
>> Log:
>> CoverageMapping: Bitvectorize code. No functionality change.
>
> This is *not* NFC.

I've noticed two uses of NFC and I'd like to mention on the mailing list 
that they should be distinguished:
   * NFC / No Functionality Change. This patch does not change anything 
visible to a user, for instance a comment-only change. You should skip 
this patch when bisecting or looking for a patch that broke something.
   * NFCI / No Functionality Change Intended. This patch rewrote 
something but ultimately should work exactly the same way for all 
well-defined inputs. Do not skip this when bisecting. Useful to 
understand why there is no test for the change (it's behaviour 
preserving!) and for reviewers to look for cases where the behaviour did 
change.

I think this patch may have been the latter, but not the former.

Nick

>> Modified:
>>      llvm/trunk/lib/ProfileData/CoverageMapping.cpp
>>
>> Modified: llvm/trunk/lib/ProfileData/CoverageMapping.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/CoverageMapping.cpp?rev=228934&r1=228933&r2=228934&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/ProfileData/CoverageMapping.cpp (original)
>> +++ llvm/trunk/lib/ProfileData/CoverageMapping.cpp Thu Feb 12 10:18:07 2015
>> @@ -15,7 +15,7 @@
>>   #include "llvm/ProfileData/CoverageMapping.h"
>>   #include "llvm/ADT/DenseMap.h"
>>   #include "llvm/ADT/Optional.h"
>> -#include "llvm/ADT/SmallSet.h"
>> +#include "llvm/ADT/SmallBitVector.h"
>>   #include "llvm/ProfileData/CoverageMappingReader.h"
>>   #include "llvm/ProfileData/InstrProfReader.h"
>>   #include "llvm/Support/Debug.h"
>> @@ -340,42 +340,35 @@ std::vector<StringRef>  CoverageMapping::
>>     return Filenames;
>>   }
>>
>> -static Optional<unsigned>  findMainViewFileID(StringRef SourceFile,
>> -                                             const FunctionRecord&Function) {
>> -  llvm::SmallVector<bool, 8>  IsExpandedFile(Function.Filenames.size(), false);
>> -  llvm::SmallVector<bool, 8>  FilenameEquivalence(Function.Filenames.size(),
>> -                                                 false);
>> +static SmallBitVector gatherFileIDs(StringRef SourceFile,
>> +                                    const FunctionRecord&Function) {
>> +  SmallBitVector FilenameEquivalence(Function.Filenames.size(), false);
>>     for (unsigned I = 0, E = Function.Filenames.size(); I<  E; ++I)
>>       if (SourceFile == Function.Filenames[I])
>>         FilenameEquivalence[I] = true;
>> +  return FilenameEquivalence;
>> +}
>> +
>> +static Optional<unsigned>  findMainViewFileID(StringRef SourceFile,
>> +                                             const FunctionRecord&Function) {
>> +  SmallBitVector IsNotExpandedFile(Function.Filenames.size(), true);
>> +  SmallBitVector FilenameEquivalence = gatherFileIDs(SourceFile, Function);
>>     for (const auto&CR : Function.CountedRegions)
>>       if (CR.Kind == CounterMappingRegion::ExpansionRegion&&
>>           FilenameEquivalence[CR.FileID])
>> -      IsExpandedFile[CR.ExpandedFileID] = true;
>> -  for (unsigned I = 0, E = Function.Filenames.size(); I<  E; ++I)
>> -    if (FilenameEquivalence[I]&&  !IsExpandedFile[I])
>> -      return I;
>> -  return None;
>> +      IsNotExpandedFile[CR.ExpandedFileID] = false;
>> +  IsNotExpandedFile&= FilenameEquivalence;
>> +  int I = IsNotExpandedFile.find_first();
>> +  return I != -1 ? I : None;
>
> You can't use None like this. Since it's just an enum value, the ternary
> here implicitly converts it to int to match I, before the return value
> is converted to Optional. Of course, this means the result is
> Optional<unsigned>(0) instead of None, which is bad.
>
> There are actually two deeper problems this points out:
>
> 1. None is implicitly convertible to int. There's never a time when we
>     want this. Maybe we should change this from "enum NoneType { None }"
>     to "static struct NoneType {} None" or so?
>
> 2. It's really unfortunate that no tests caught this. I've been
>     improving the testing situation here lately, so maybe it's not hard
>     to add a unittest now.
>
>>   }
>>
>>   static Optional<unsigned>  findMainViewFileID(const FunctionRecord&Function) {
>> -  llvm::SmallVector<bool, 8>  IsExpandedFile(Function.Filenames.size(), false);
>> +  SmallBitVector IsNotExpandedFile(Function.Filenames.size(), false);
>>     for (const auto&CR : Function.CountedRegions)
>>       if (CR.Kind == CounterMappingRegion::ExpansionRegion)
>> -      IsExpandedFile[CR.ExpandedFileID] = true;
>> -  for (unsigned I = 0, E = Function.Filenames.size(); I<  E; ++I)
>> -    if (!IsExpandedFile[I])
>> -      return I;
>> -  return None;
>> -}
>> -
>> -static SmallSet<unsigned, 8>  gatherFileIDs(StringRef SourceFile,
>> -                                           const FunctionRecord&Function) {
>> -  SmallSet<unsigned, 8>  IDs;
>> -  for (unsigned I = 0, E = Function.Filenames.size(); I<  E; ++I)
>> -    if (SourceFile == Function.Filenames[I])
>> -      IDs.insert(I);
>> -  return IDs;
>> +      IsNotExpandedFile[CR.ExpandedFileID] = true;
>> +  int I = IsNotExpandedFile.find_first();
>> +  return I != -1 ? I : None;
>>   }
>>
>>   /// Sort a nested sequence of regions from a single file.
>> @@ -403,7 +396,7 @@ CoverageData CoverageMapping::getCoverag
>>         continue;
>>       auto FileIDs = gatherFileIDs(Filename, Function);
>>       for (const auto&CR : Function.CountedRegions)
>> -      if (FileIDs.count(CR.FileID)) {
>> +      if (FileIDs.test(CR.FileID)) {
>>           Regions.push_back(CR);
>>           if (isExpansion(CR, *MainFileID))
>>             FileCoverage.Expansions.emplace_back(CR, Function);
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>