[llvm] r284887 - Switch SmallSetVector to use DenseSet when it overflows its inline space.
Justin Lebar via llvm-commits
llvm-commits at lists.llvm.org
Sun Oct 23 10:56:59 PDT 2016
https://reviews.llvm.org/D25900
On Sun, Oct 23, 2016 at 12:21 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>
>> On Oct 22, 2016, at 11:06 PM, Justin Lebar <jlebar at google.com> wrote:
>>
>>> LLVM is fine in using only power of 2 sizes, but we don’t know about external users.
>>
>> I'm happy to let them speak for themselves -- so far you're the first
>> to complain.
>>
>> There's prior art in the other classes just to round the stated size
>> up to the next power of two. How does doing that here sound to you?
>
> Works for me.
>
>>
>> On Sat, Oct 22, 2016 at 10:01 PM, Quentin Colombet <qcolombet at apple.com> 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
>>>
>
More information about the llvm-commits
mailing list