[llvm] r211080 - Expose ValueMap's mutex type as a typedef instead of a sys::Mutex.
Zachary Turner
zturner at google.com
Tue Jun 17 10:53:14 PDT 2014
If you want a hint, everywhere is a recursive mutex except, as far as I can
tell, in the ValueMapTest in unittests, which uses a non recursive mutex.
Because recursive seems to be the most common, i made it the default
template argument type so that an existing user will not have to change his
syntax to compile with the new template arg.
Additionally, having a template argument here allows one to use a dummy
mutex that no-ops all its methods. Currently we return nullptr if there's
no mutex, but this would open up the possibility of returning a reference
to a mutex and if someone doesn't want locking, they can simply specify
null_mutex. I recall that the pattern
if (lock)
lock->lock();
...
if (lock)
lock->unlock();
is difficult for a compiler to deal with (i don't remember the details, I
just remember Herb Sutter once saying that MSVC generates / generated
incorrect code with this pattern with certian types of optimization
enabled), so it would fix that as well.
On Tue, Jun 17, 2014 at 10:47 AM, David Blaikie <dblaikie at gmail.com> wrote:
> On Tue, Jun 17, 2014 at 9:14 AM, Zachary Turner <zturner at google.com>
> wrote:
> > This is so I can change smaller amounts of code incrementally, but with
> > sys::Mutex, whether or not the mutex is recursive is a runtime parameter
> to
> > the constructor, and with the std mutexes, recursive and non-recursive
> > mutexes are entirely different types. It turns out that ValueMap is
> being
> > used with both recursive and non-recursive mutexes, so the only way to
> > support this is to templatize the mutex type.
>
> OK - I'll keep an eye out for the follow up patches to get an idea of
> how/why/where different users specify different kinds of mutexes.
> Thanks for the details
>
> - David
>
> >
> >
> > On Mon, Jun 16, 2014 at 10:38 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>
> >> I'm assuming this is so you can change smaller amounts of code/work
> >> incrementally, and at the end everything will be std::murex and this new
> >> template parameter can be removed?
> >>
> >> On Jun 16, 2014 5:27 PM, "Zachary Turner" <zturner at google.com> wrote:
> >>>
> >>> Author: zturner
> >>> Date: Mon Jun 16 19:17:38 2014
> >>> New Revision: 211080
> >>>
> >>> URL: http://llvm.org/viewvc/llvm-project?rev=211080&view=rev
> >>> Log:
> >>> Expose ValueMap's mutex type as a typedef instead of a sys::Mutex.
> >>>
> >>> This enables static polymorphism of the mutex type, which is
> >>> necessary in order to replace the standard mutex implementation
> >>> with a different type.
> >>>
> >>> Modified:
> >>> llvm/trunk/include/llvm/IR/ValueMap.h
> >>> llvm/trunk/unittests/IR/ValueMapTest.cpp
> >>>
> >>> Modified: llvm/trunk/include/llvm/IR/ValueMap.h
> >>> URL:
> >>>
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/ValueMap.h?rev=211080&r1=211079&r2=211080&view=diff
> >>>
> >>>
> ==============================================================================
> >>> --- llvm/trunk/include/llvm/IR/ValueMap.h (original)
> >>> +++ llvm/trunk/include/llvm/IR/ValueMap.h Mon Jun 16 19:17:38 2014
> >>> @@ -45,8 +45,10 @@ class ValueMapConstIterator;
> >>> /// This class defines the default behavior for configurable aspects
> of
> >>> /// ValueMap<>. User Configs should inherit from this class to be as
> >>> compatible
> >>> /// as possible with future versions of ValueMap.
> >>> -template<typename KeyT>
> >>> +template<typename KeyT, typename MutexT = sys::Mutex>
> >>> struct ValueMapConfig {
> >>> + typedef MutexT mutex_type;
> >>> +
> >>> /// If FollowRAUW is true, the ValueMap will update mappings on
> RAUW.
> >>> If it's
> >>> /// false, the ValueMap will leave the original mapping in place.
> >>> enum { FollowRAUW = true };
> >>> @@ -67,7 +69,7 @@ struct ValueMapConfig {
> >>> /// and onDelete) and not inside other ValueMap methods. NULL means
> >>> that no
> >>> /// mutex is necessary.
> >>> template<typename ExtraDataT>
> >>> - static sys::Mutex *getMutex(const ExtraDataT &/*Data*/) { return
> >>> nullptr; }
> >>> + static mutex_type *getMutex(const ExtraDataT &/*Data*/) { return
> >>> nullptr; }
> >>> };
> >>>
> >>> /// See the file comment.
> >>> @@ -212,7 +214,7 @@ public:
> >>> void deleted() override {
> >>> // Make a copy that won't get changed even when *this is
> destroyed.
> >>> ValueMapCallbackVH Copy(*this);
> >>> - sys::Mutex *M = Config::getMutex(Copy.Map->Data);
> >>> + typename Config::mutex_type *M = Config::getMutex(Copy.Map->Data);
> >>> if (M)
> >>> M->acquire();
> >>> Config::onDelete(Copy.Map->Data, Copy.Unwrap()); // May destroy
> >>> *this.
> >>> @@ -225,7 +227,7 @@ public:
> >>> "Invalid RAUW on key of ValueMap<>");
> >>> // Make a copy that won't get changed even when *this is
> destroyed.
> >>> ValueMapCallbackVH Copy(*this);
> >>> - sys::Mutex *M = Config::getMutex(Copy.Map->Data);
> >>> + typename Config::mutex_type *M = Config::getMutex(Copy.Map->Data);
> >>> if (M)
> >>> M->acquire();
> >>>
> >>>
> >>> Modified: llvm/trunk/unittests/IR/ValueMapTest.cpp
> >>> URL:
> >>>
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/ValueMapTest.cpp?rev=211080&r1=211079&r2=211080&view=diff
> >>>
> >>>
> ==============================================================================
> >>> --- llvm/trunk/unittests/IR/ValueMapTest.cpp (original)
> >>> +++ llvm/trunk/unittests/IR/ValueMapTest.cpp Mon Jun 16 19:17:38 2014
> >>> @@ -177,10 +177,10 @@ TYPED_TEST(ValueMapTest, ConfiguredColli
> >>> // TODO: Implement this when someone needs it.
> >>> }
> >>>
> >>> -template<typename KeyT>
> >>> -struct LockMutex : ValueMapConfig<KeyT> {
> >>> +template<typename KeyT, typename MutexT>
> >>> +struct LockMutex : ValueMapConfig<KeyT, MutexT> {
> >>> struct ExtraData {
> >>> - sys::Mutex *M;
> >>> + mutex_type *M;
> >>> bool *CalledRAUW;
> >>> bool *CalledDeleted;
> >>> };
> >>> @@ -192,15 +192,15 @@ struct LockMutex : ValueMapConfig<KeyT>
> >>> *Data.CalledDeleted = true;
> >>> EXPECT_FALSE(Data.M->tryacquire()) << "Mutex should already be
> >>> locked.";
> >>> }
> >>> - static sys::Mutex *getMutex(const ExtraData &Data) { return Data.M;
> }
> >>> + static mutex_type *getMutex(const ExtraData &Data) { return Data.M;
> }
> >>> };
> >>> #if LLVM_ENABLE_THREADS
> >>> TYPED_TEST(ValueMapTest, LocksMutex) {
> >>> sys::Mutex M(false); // Not recursive.
> >>> bool CalledRAUW = false, CalledDeleted = false;
> >>> - typename LockMutex<TypeParam*>::ExtraData Data =
> >>> - {&M, &CalledRAUW, &CalledDeleted};
> >>> - ValueMap<TypeParam*, int, LockMutex<TypeParam*> > VM(Data);
> >>> + typedef LockMutex<TypeParam*, sys::Mutex> ConfigType;
> >>> + typename ConfigType::ExtraData Data = {&M, &CalledRAUW,
> >>> &CalledDeleted};
> >>> + ValueMap<TypeParam*, int, ConfigType> VM(Data);
> >>> VM[this->BitcastV.get()] = 7;
> >>> this->BitcastV->replaceAllUsesWith(this->AddV.get());
> >>> this->AddV.reset();
> >>>
> >>>
> >>> _______________________________________________
> >>> llvm-commits mailing list
> >>> llvm-commits at cs.uiuc.edu
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140617/b3f3b287/attachment.html>
More information about the llvm-commits
mailing list