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

[Justin Bogner]

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.

> 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