[llvm] r211080 - Expose ValueMap's mutex type as a typedef instead of a sys::Mutex.
David Blaikie
dblaikie at gmail.com
Tue Jun 17 11:11:13 PDT 2014
On Tue, Jun 17, 2014 at 10:53 AM, Zachary Turner <zturner at google.com> wrote:
> 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.
That probably wouldn't meet my bar for necessary complexity. I assume
the test could work just fine with a 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.
Seems reasonable.
> 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.
Could be handy.
> 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.
That's disturbing. I wouldn't be sure if the change in API to a null
mutex would help, but I suppose there's a pretty good chance it
could...
>
>
> 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
>> >
>> >
>
>
More information about the llvm-commits
mailing list