[llvm] r221960 - IR: Rewrite uniquing and creation of MDString
David Blaikie
dblaikie at gmail.com
Tue Nov 18 10:06:37 PST 2014
On Mon, Nov 17, 2014 at 3:27 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > On 2014-Nov-17, at 11:26, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Thu, Nov 13, 2014 at 5:17 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > Author: dexonsmith
> > Date: Thu Nov 13 19:17:09 2014
> > New Revision: 221960
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=221960&view=rev
> > Log:
> > IR: Rewrite uniquing and creation of MDString
> >
> > Stop using `Value::getName()` to get the string behind an `MDString`.
> > Switch to `StringMapEntry<MDString>` so that we can find the string by
> > its coallocation.
> >
> > This is part of PR21532.
> >
> > Modified:
> > llvm/trunk/include/llvm/IR/Metadata.h
> > llvm/trunk/lib/IR/LLVMContextImpl.cpp
> > llvm/trunk/lib/IR/LLVMContextImpl.h
> > llvm/trunk/lib/IR/Metadata.cpp
> >
> > Modified: llvm/trunk/include/llvm/IR/Metadata.h
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Metadata.h?rev=221960&r1=221959&r2=221960&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/include/llvm/IR/Metadata.h (original)
> > +++ llvm/trunk/include/llvm/IR/Metadata.h Thu Nov 13 19:17:09 2014
> > @@ -57,14 +57,15 @@ public:
> > ///
> > /// TODO: Inherit from Metadata.
> > class MDString : public Value {
> > + friend class StringMapEntry<MDString>;
> >
> > This seems annoyingly tied to the implementation details of StringMap
> (exactly where in the stack the constructor is called, etc)... I wonder if
> there's a better answer, but none springs immediately to mind.
> >
> > +
> > virtual void anchor();
> > MDString(const MDString &) LLVM_DELETED_FUNCTION;
> >
> > explicit MDString(LLVMContext &C);
> >
> > -private:
> > /// \brief Shadow Value::getName() to prevent its use.
> > - StringRef getName() const { return Value::getName(); }
> > + StringRef getName() const LLVM_DELETED_FUNCTION;
> >
> > public:
> > static MDString *get(LLVMContext &Context, StringRef Str);
> > @@ -72,17 +73,17 @@ public:
> > return get(Context, Str ? StringRef(Str) : StringRef());
> > }
> >
> > - StringRef getString() const { return getName(); }
> > + StringRef getString() const;
> >
> > - unsigned getLength() const { return (unsigned)getName().size(); }
> > + unsigned getLength() const { return (unsigned)getString().size(); }
> >
> > typedef StringRef::iterator iterator;
> >
> > /// \brief Pointer to the first byte of the string.
> > - iterator begin() const { return getName().begin(); }
> > + iterator begin() const { return getString().begin(); }
> >
> > /// \brief Pointer to one byte past the end of the string.
> > - iterator end() const { return getName().end(); }
> > + iterator end() const { return getString().end(); }
> >
> > /// \brief Methods for support type inquiry through isa, cast, and
> dyn_cast.
> > static bool classof(const Value *V) {
> >
> > Modified: llvm/trunk/lib/IR/LLVMContextImpl.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/LLVMContextImpl.cpp?rev=221960&r1=221959&r2=221960&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/IR/LLVMContextImpl.cpp (original)
> > +++ llvm/trunk/lib/IR/LLVMContextImpl.cpp Thu Nov 13 19:17:09 2014
> > @@ -135,7 +135,7 @@ LLVMContextImpl::~LLVMContextImpl() {
> > "Destroying all MDNodes didn't empty the Context's sets.");
> >
> > // Destroy MDStrings.
> > - DeleteContainerSeconds(MDStringCache);
> > + MDStringCache.clear();
> > }
> >
> > // ConstantsContext anchors
> >
> > Modified: llvm/trunk/lib/IR/LLVMContextImpl.h
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/LLVMContextImpl.h?rev=221960&r1=221959&r2=221960&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/IR/LLVMContextImpl.h (original)
> > +++ llvm/trunk/lib/IR/LLVMContextImpl.h Thu Nov 13 19:17:09 2014
> > @@ -261,7 +261,7 @@ public:
> > FoldingSet<AttributeSetImpl> AttrsLists;
> > FoldingSet<AttributeSetNode> AttrsSetNodes;
> >
> > - StringMap<Value*> MDStringCache;
> > + StringMap<MDString> MDStringCache;
> >
> > FoldingSet<MDNode> MDNodeSet;
> >
> >
> > Modified: llvm/trunk/lib/IR/Metadata.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Metadata.cpp?rev=221960&r1=221959&r2=221960&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/IR/Metadata.cpp (original)
> > +++ llvm/trunk/lib/IR/Metadata.cpp Thu Nov 13 19:17:09 2014
> > @@ -37,13 +37,21 @@ MDString::MDString(LLVMContext &C)
> > : Value(Type::getMetadataTy(C), Value::MDStringVal) {}
> >
> > MDString *MDString::get(LLVMContext &Context, StringRef Str) {
> > - LLVMContextImpl *pImpl = Context.pImpl;
> > - StringMapEntry<Value*> &Entry =
> > - pImpl->MDStringCache.GetOrCreateValue(Str);
> > - Value *&S = Entry.getValue();
> > - if (!S) S = new MDString(Context);
> > - S->setValueName(&Entry);
> > - return cast<MDString>(S);
> > + 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(),
> Context);
> >
> > This seems like it's the wrong API design - forwarding a single argument
> and calling an explicit constructor doesn't seem right. APIs (such as
> push_back) generally do perfect forwarding of a single argument where the
> argument is implicitly convertible to the underlying type, but not for
> explicit conversions.
> >
> > For explicit conversions the idiom seems to be to have an 'emplace'
> function.
> >
> > I think the right API design is to add an 'emplace' to StringMap and use
> it something like this:
> >
> > Store.insert(std::piecewise_construct, std::forward_as_tuple(Str),
> std::forward_as_tuple(Context));
> >
>
> `std::forward_as_tuple()` and `std::piecewise_construct` are new to me,
> thanks for suggesting it. I see that you're using this in
> LexicalScopes.cpp.
>
> It's not clear to me how to unpack the tuple to send into a
> constructor when we're not leveraging `std::pair<>`. I had a look at
> the libcxx implementation of `utility`, and it seems to use an
> unexposed class called `__tuple_indices` to do the magic.
>
Unexposed, but not magic - it can be implemented in LLVM as it is in
libc++. A few of these utilities are a bit of a pain to write, but they're
generic enough not to need to be written very often.
> Note that `StringMap` doesn't actually use a `std::pair<>`.
> `StringMapEntry` has a similar API, but is distinct.
>
> Am I missing something obvious?
>
StringMap still exposes an Associative Container interface in many places
(like its insert(std::pair<StringRef, ValueTy>) rather than
insert(StringRef, ValueTy)) so it seems reasonable to provide an emplace
with a similar API.
> Nevertheless, StringMap is pretty simple. We don't care about
> forwarding the key, just the value. *IF* we were allowed to use
> variadic templates, it'd be easy enough to implement:
>
> template <class... ArgsT>
> std::pair<iterator, bool> emplace(StringRef Key, ArgsT &&...Args);
>
> This was my original plan, but then I remembered (incorrectly?) that
> MSVC doesn't support variadic templates yet, so I just hacked in
> something that works.
MSVC doesn't support variadic templates in the minimum version of MSVC we
support, so we fake out variadics whenever we need to (see the existing
uses of LLVM_HAS_VARIADIC_TEMPLATES in LLVM).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141118/81d85b8b/attachment.html>
More information about the llvm-commits
mailing list