[llvm] r284887 - Switch SmallSetVector to use DenseSet when it overflows its inline space.

Quentin Colombet via llvm-commits llvm-commits at lists.llvm.org
Sat Oct 22 22:01:43 PDT 2016


Hi Justin,

This change has a nasty side effect that we should either document, or revert to the previous implementation.
The DenseSet class has a requirement that the size of the initial bucket must be a power of two.

Therefore, if you create a SmallSectVector for a non-power of two, you get an assertion later on in the initialization of the DenseMap.
In release mode, the compiler crashes in weird way, but at this point that’s expected.
If you want to reproduce the problem, just create a small program with a SmallSetVector of 5.

I am not sure I like that we impose this limitation on the size but given we already do on the DenseMap I can live with that.
At the very least, this change should be documented in the API.

LLVM is fine in using only power of 2 sizes, but we don’t know about external users.

Cheers,
-Quentin

> On Oct 21, 2016, at 2:45 PM, Justin Lebar via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> Author: jlebar
> Date: Fri Oct 21 16:45:01 2016
> New Revision: 284887
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=284887&view=rev
> Log:
> Switch SmallSetVector to use DenseSet when it overflows its inline space.
> 
> Summary:
> SetVector already used DenseSet, but SmallSetVector used std::set.  This
> leads to surprising performance differences.  Moreover, it means that
> the set of key types accepted by SetVector and SmallSetVector are
> quite different!
> 
> In order to make this change, we had to convert some callsites that used
> SmallSetVector<std::string, N> to use SmallSetVector<CachedHashString, N>
> instead.
> 
> Reviewers: timshen
> 
> Subscribers: llvm-commits
> 
> Differential Revision: https://reviews.llvm.org/D25648
> 
> Modified:
>    llvm/trunk/include/llvm/ADT/SetVector.h
>    llvm/trunk/utils/TableGen/AsmMatcherEmitter.cpp
>    llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp
> 
> Modified: llvm/trunk/include/llvm/ADT/SetVector.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SetVector.h?rev=284887&r1=284886&r2=284887&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/SetVector.h (original)
> +++ llvm/trunk/include/llvm/ADT/SetVector.h Fri Oct 21 16:45:01 2016
> @@ -282,7 +282,8 @@ private:
> /// \brief A SetVector that performs no allocations if smaller than
> /// a certain size.
> template <typename T, unsigned N>
> -class SmallSetVector : public SetVector<T, SmallVector<T, N>, SmallSet<T, N> > {
> +class SmallSetVector
> +    : public SetVector<T, SmallVector<T, N>, SmallDenseSet<T, N>> {
> public:
>   SmallSetVector() {}
> 
> 
> Modified: llvm/trunk/utils/TableGen/AsmMatcherEmitter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/AsmMatcherEmitter.cpp?rev=284887&r1=284886&r2=284887&view=diff
> ==============================================================================
> --- llvm/trunk/utils/TableGen/AsmMatcherEmitter.cpp (original)
> +++ llvm/trunk/utils/TableGen/AsmMatcherEmitter.cpp Fri Oct 21 16:45:01 2016
> @@ -97,6 +97,7 @@
> //===----------------------------------------------------------------------===//
> 
> #include "CodeGenTarget.h"
> +#include "llvm/ADT/CachedHashString.h"
> #include "llvm/ADT/PointerUnion.h"
> #include "llvm/ADT/STLExtras.h"
> #include "llvm/ADT/SmallPtrSet.h"
> @@ -1822,10 +1823,11 @@ void MatchableInfo::buildAliasResultOper
>   }
> }
> 
> -static unsigned getConverterOperandID(const std::string &Name,
> -                                      SmallSetVector<std::string, 16> &Table,
> -                                      bool &IsNew) {
> -  IsNew = Table.insert(Name);
> +static unsigned
> +getConverterOperandID(const std::string &Name,
> +                      SmallSetVector<CachedHashString, 16> &Table,
> +                      bool &IsNew) {
> +  IsNew = Table.insert(CachedHashString(Name));
> 
>   unsigned ID = IsNew ? Table.size() - 1 : find(Table, Name) - Table.begin();
> 
> @@ -1838,8 +1840,8 @@ static void emitConvertFuncs(CodeGenTarg
>                              std::vector<std::unique_ptr<MatchableInfo>> &Infos,
>                              bool HasMnemonicFirst, bool HasOptionalOperands,
>                              raw_ostream &OS) {
> -  SmallSetVector<std::string, 16> OperandConversionKinds;
> -  SmallSetVector<std::string, 16> InstructionConversionKinds;
> +  SmallSetVector<CachedHashString, 16> OperandConversionKinds;
> +  SmallSetVector<CachedHashString, 16> InstructionConversionKinds;
>   std::vector<std::vector<uint8_t> > ConversionTable;
>   size_t MaxRowLength = 2; // minimum is custom converter plus terminator.
> 
> @@ -1911,9 +1913,9 @@ static void emitConvertFuncs(CodeGenTarg
> 
>   // Pre-populate the operand conversion kinds with the standard always
>   // available entries.
> -  OperandConversionKinds.insert("CVT_Done");
> -  OperandConversionKinds.insert("CVT_Reg");
> -  OperandConversionKinds.insert("CVT_Tied");
> +  OperandConversionKinds.insert(CachedHashString("CVT_Done"));
> +  OperandConversionKinds.insert(CachedHashString("CVT_Reg"));
> +  OperandConversionKinds.insert(CachedHashString("CVT_Tied"));
>   enum { CVT_Done, CVT_Reg, CVT_Tied };
> 
>   for (auto &II : Infos) {
> @@ -1925,13 +1927,13 @@ static void emitConvertFuncs(CodeGenTarg
>       II->ConversionFnKind = Signature;
> 
>       // Check if we have already generated this signature.
> -      if (!InstructionConversionKinds.insert(Signature))
> +      if (!InstructionConversionKinds.insert(CachedHashString(Signature)))
>         continue;
> 
>       // Remember this converter for the kind enum.
>       unsigned KindID = OperandConversionKinds.size();
> -      OperandConversionKinds.insert("CVT_" +
> -                                    getEnumNameForToken(AsmMatchConverter));
> +      OperandConversionKinds.insert(
> +          CachedHashString("CVT_" + getEnumNameForToken(AsmMatchConverter)));
> 
>       // Add the converter row for this instruction.
>       ConversionTable.emplace_back();
> @@ -2113,7 +2115,7 @@ static void emitConvertFuncs(CodeGenTarg
> 
>     // Save the signature. If we already have it, don't add a new row
>     // to the table.
> -    if (!InstructionConversionKinds.insert(Signature))
> +    if (!InstructionConversionKinds.insert(CachedHashString(Signature)))
>       continue;
> 
>     // Add the row to the table.
> @@ -2130,14 +2132,14 @@ static void emitConvertFuncs(CodeGenTarg
> 
>   // Output the operand conversion kind enum.
>   OS << "enum OperatorConversionKind {\n";
> -  for (const std::string &Converter : OperandConversionKinds)
> +  for (const auto &Converter : OperandConversionKinds)
>     OS << "  " << Converter << ",\n";
>   OS << "  CVT_NUM_CONVERTERS\n";
>   OS << "};\n\n";
> 
>   // Output the instruction conversion kind enum.
>   OS << "enum InstructionConversionKind {\n";
> -  for (const std::string &Signature : InstructionConversionKinds)
> +  for (const auto &Signature : InstructionConversionKinds)
>     OS << "  " << Signature << ",\n";
>   OS << "  CVT_NUM_SIGNATURES\n";
>   OS << "};\n\n";
> 
> Modified: llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp?rev=284887&r1=284886&r2=284887&view=diff
> ==============================================================================
> --- llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp (original)
> +++ llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp Fri Oct 21 16:45:01 2016
> @@ -14,6 +14,7 @@
> 
> #include "CodeGenTarget.h"
> #include "llvm/ADT/APInt.h"
> +#include "llvm/ADT/CachedHashString.h"
> #include "llvm/ADT/SmallString.h"
> #include "llvm/ADT/StringExtras.h"
> #include "llvm/ADT/StringRef.h"
> @@ -66,8 +67,8 @@ typedef std::vector<uint8_t> DecoderTabl
> typedef uint32_t DecoderFixup;
> typedef std::vector<DecoderFixup> FixupList;
> typedef std::vector<FixupList> FixupScopeList;
> -typedef SmallSetVector<std::string, 16> PredicateSet;
> -typedef SmallSetVector<std::string, 16> DecoderSet;
> +typedef SmallSetVector<CachedHashString, 16> PredicateSet;
> +typedef SmallSetVector<CachedHashString, 16> DecoderSet;
> struct DecoderTableInfo {
>   DecoderTable Table;
>   FixupScopeList FixupStack;
> @@ -1106,7 +1107,7 @@ unsigned FilterChooser::getDecoderIndex(
>   // overkill for now, though.
> 
>   // Make sure the predicate is in the table.
> -  Decoders.insert(StringRef(Decoder));
> +  Decoders.insert(CachedHashString(Decoder));
>   // Now figure out the index for when we write out the table.
>   DecoderSet::const_iterator P = find(Decoders, Decoder.str());
>   return (unsigned)(P - Decoders.begin());
> @@ -1179,9 +1180,9 @@ unsigned FilterChooser::getPredicateInde
>   // overkill for now, though.
> 
>   // Make sure the predicate is in the table.
> -  TableInfo.Predicates.insert(Predicate.str());
> +  TableInfo.Predicates.insert(CachedHashString(Predicate));
>   // Now figure out the index for when we write out the table.
> -  PredicateSet::const_iterator P = find(TableInfo.Predicates, Predicate.str());
> +  PredicateSet::const_iterator P = find(TableInfo.Predicates, Predicate);
>   return (unsigned)(P - TableInfo.Predicates.begin());
> }
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits



More information about the llvm-commits mailing list