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

[Mehdi Amini via llvm-commits]

> 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 <mailto: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 <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?
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 <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <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/8109c707/attachment.html>