[PATCH] D16597: Query the StringMap only once when creating MDString (NFC)

[David Blaikie via llvm-commits]

On Tue, Jan 26, 2016 at 1:40 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:

>
> On Jan 26, 2016, at 1:30 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Tue, Jan 26, 2016 at 12:34 PM, Mehdi AMINI via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> joker.eph created this revision.
>> joker.eph added a reviewer: dexonsmith.
>> joker.eph added a subscriber: llvm-commits.
>>
>> Loading IR with debug info improves MDString::get() from 19ms to 10ms.
>>
>> http://reviews.llvm.org/D16597
>>
>> Files:
>>   include/llvm/IR/Metadata.h
>>   lib/IR/Metadata.cpp
>>
>> Index: lib/IR/Metadata.cpp
>> ===================================================================
>> --- lib/IR/Metadata.cpp
>> +++ lib/IR/Metadata.cpp
>> @@ -399,17 +399,12 @@
>>
>>  MDString *MDString::get(LLVMContext &Context, StringRef Str) {
>>    auto &Store = Context.pImpl->MDStringCache;
>> -  auto I = Store.find(Str);
>> -  if (I != Store.end())
>> -    return &I->second;
>> -
>> -  auto *Entry =
>> -      StringMapEntry<MDString>::Create(Str, Store.getAllocator(),
>> MDString());
>> -  bool WasInserted = Store.insert(Entry);
>> -  (void)WasInserted;
>> -  assert(WasInserted && "Expected entry to be inserted");
>> -  Entry->second.Entry = Entry;
>> -  return &Entry->second;
>> +  auto I = Store.insert(std::make_pair(Str, MDString()));
>> +  auto &MapEntry = I.first->getValue();
>> +  if (!I.second)
>> +    return &MapEntry;
>> +  MapEntry.Entry = &*I.first;
>> +  return &MapEntry;
>>  }
>>
>>  StringRef MDString::getString() const {
>> Index: include/llvm/IR/Metadata.h
>> ===================================================================
>> --- include/llvm/IR/Metadata.h
>> +++ include/llvm/IR/Metadata.h
>> @@ -599,9 +599,9 @@
>>
>>    StringMapEntry<MDString> *Entry;
>>    MDString() : Metadata(MDStringKind, Uniqued), Entry(nullptr) {}
>> -  MDString(MDString &&) : Metadata(MDStringKind, Uniqued) {}
>>
>
> Well that was weird/scary/bogus ^
>
>
> Yes but private :)
>
>
>
>>
>>  public:
>> +  MDString(MDString &&R) : Metadata(MDStringKind, Uniqued),
>> Entry(R.Entry) {}
>>
>
> Could we just remove this, and rely on the implicit move/copy ctors? ^ I
> guess maybe the base copy/move ctor doesn't do the right thing (since it's
> not being used here)
>
>
> Good point!
>
>
> Should we just make this the copy ctor? It doesn't seme to really be
> moving anything.
>
>
> We could consider MDString to be a “lightweight” value-based type (like
> StringRef) and declare a copy constructor, but will it be coherent with the
> rest of the other metadata types?
>

Not sure about the rest of them, but it seems reasonable for this one (if
we're going to amke it public at all - at that point we admit that MDString
is much like StringRef, as you say)


> I was not really happy to make it public in the first place, but std::pair
> needs it to insert in the map.
>
> —
> Mehdi
>
>
>
>
>
>
>>    static MDString *get(LLVMContext &Context, StringRef Str);
>>    static MDString *get(LLVMContext &Context, const char *Str) {
>>      return get(Context, Str ? StringRef(Str) : StringRef());
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160126/68879216/attachment.html>