[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:10:55 PDT 2016
> On Oct 22, 2016, at 10:01 PM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> 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.
To be clear though, I would rather support the previous behavior with non-power of 2.
>
> 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
>
> _______________________________________________
> 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