[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 23:00:51 PDT 2016
I believe right now this is passed directly via a template, i.e., constant argument.
We could probably do something statically, but I was afraid it starts to look ugly.
> On Oct 22, 2016, at 10:57 PM, escha at apple.com wrote:
>
> Why not round up the initial bucket size to the nearest power of 2 when converting to a DenseMap?
>
> —escha
>
>> 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.
>>
>> 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