<div dir="ltr">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.<div>
<br></div><div>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</div>
<div><br></div><div>if (lock)</div><div> lock->lock();</div><div><br></div><div>...</div><div><br></div><div>if (lock)</div><div> lock->unlock();</div><div><br></div><div>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.</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jun 17, 2014 at 10:47 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On Tue, Jun 17, 2014 at 9:14 AM, Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br>
> This is so I can change smaller amounts of code incrementally, but with<br>
> sys::Mutex, whether or not the mutex is recursive is a runtime parameter to<br>
> the constructor, and with the std mutexes, recursive and non-recursive<br>
> mutexes are entirely different types. It turns out that ValueMap is being<br>
> used with both recursive and non-recursive mutexes, so the only way to<br>
> support this is to templatize the mutex type.<br>
<br>
</div>OK - I'll keep an eye out for the follow up patches to get an idea of<br>
how/why/where different users specify different kinds of mutexes.<br>
Thanks for the details<br>
<span class="HOEnZb"><font color="#888888"><br>
- David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> On Mon, Jun 16, 2014 at 10:38 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> I'm assuming this is so you can change smaller amounts of code/work<br>
>> incrementally, and at the end everything will be std::murex and this new<br>
>> template parameter can be removed?<br>
>><br>
>> On Jun 16, 2014 5:27 PM, "Zachary Turner" <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br>
>>><br>
>>> Author: zturner<br>
>>> Date: Mon Jun 16 19:17:38 2014<br>
>>> New Revision: 211080<br>
>>><br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=211080&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=211080&view=rev</a><br>
>>> Log:<br>
>>> Expose ValueMap's mutex type as a typedef instead of a sys::Mutex.<br>
>>><br>
>>> This enables static polymorphism of the mutex type, which is<br>
>>> necessary in order to replace the standard mutex implementation<br>
>>> with a different type.<br>
>>><br>
>>> Modified:<br>
>>> llvm/trunk/include/llvm/IR/ValueMap.h<br>
>>> llvm/trunk/unittests/IR/ValueMapTest.cpp<br>
>>><br>
>>> Modified: llvm/trunk/include/llvm/IR/ValueMap.h<br>
>>> URL:<br>
>>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/ValueMap.h?rev=211080&r1=211079&r2=211080&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/ValueMap.h?rev=211080&r1=211079&r2=211080&view=diff</a><br>
>>><br>
>>> ==============================================================================<br>
>>> --- llvm/trunk/include/llvm/IR/ValueMap.h (original)<br>
>>> +++ llvm/trunk/include/llvm/IR/ValueMap.h Mon Jun 16 19:17:38 2014<br>
>>> @@ -45,8 +45,10 @@ class ValueMapConstIterator;<br>
>>> /// This class defines the default behavior for configurable aspects of<br>
>>> /// ValueMap<>. User Configs should inherit from this class to be as<br>
>>> compatible<br>
>>> /// as possible with future versions of ValueMap.<br>
>>> -template<typename KeyT><br>
>>> +template<typename KeyT, typename MutexT = sys::Mutex><br>
>>> struct ValueMapConfig {<br>
>>> + typedef MutexT mutex_type;<br>
>>> +<br>
>>> /// If FollowRAUW is true, the ValueMap will update mappings on RAUW.<br>
>>> If it's<br>
>>> /// false, the ValueMap will leave the original mapping in place.<br>
>>> enum { FollowRAUW = true };<br>
>>> @@ -67,7 +69,7 @@ struct ValueMapConfig {<br>
>>> /// and onDelete) and not inside other ValueMap methods. NULL means<br>
>>> that no<br>
>>> /// mutex is necessary.<br>
>>> template<typename ExtraDataT><br>
>>> - static sys::Mutex *getMutex(const ExtraDataT &/*Data*/) { return<br>
>>> nullptr; }<br>
>>> + static mutex_type *getMutex(const ExtraDataT &/*Data*/) { return<br>
>>> nullptr; }<br>
>>> };<br>
>>><br>
>>> /// See the file comment.<br>
>>> @@ -212,7 +214,7 @@ public:<br>
>>> void deleted() override {<br>
>>> // Make a copy that won't get changed even when *this is destroyed.<br>
>>> ValueMapCallbackVH Copy(*this);<br>
>>> - sys::Mutex *M = Config::getMutex(Copy.Map->Data);<br>
>>> + typename Config::mutex_type *M = Config::getMutex(Copy.Map->Data);<br>
>>> if (M)<br>
>>> M->acquire();<br>
>>> Config::onDelete(Copy.Map->Data, Copy.Unwrap()); // May destroy<br>
>>> *this.<br>
>>> @@ -225,7 +227,7 @@ public:<br>
>>> "Invalid RAUW on key of ValueMap<>");<br>
>>> // Make a copy that won't get changed even when *this is destroyed.<br>
>>> ValueMapCallbackVH Copy(*this);<br>
>>> - sys::Mutex *M = Config::getMutex(Copy.Map->Data);<br>
>>> + typename Config::mutex_type *M = Config::getMutex(Copy.Map->Data);<br>
>>> if (M)<br>
>>> M->acquire();<br>
>>><br>
>>><br>
>>> Modified: llvm/trunk/unittests/IR/ValueMapTest.cpp<br>
>>> URL:<br>
>>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/ValueMapTest.cpp?rev=211080&r1=211079&r2=211080&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/ValueMapTest.cpp?rev=211080&r1=211079&r2=211080&view=diff</a><br>
>>><br>
>>> ==============================================================================<br>
>>> --- llvm/trunk/unittests/IR/ValueMapTest.cpp (original)<br>
>>> +++ llvm/trunk/unittests/IR/ValueMapTest.cpp Mon Jun 16 19:17:38 2014<br>
>>> @@ -177,10 +177,10 @@ TYPED_TEST(ValueMapTest, ConfiguredColli<br>
>>> // TODO: Implement this when someone needs it.<br>
>>> }<br>
>>><br>
>>> -template<typename KeyT><br>
>>> -struct LockMutex : ValueMapConfig<KeyT> {<br>
>>> +template<typename KeyT, typename MutexT><br>
>>> +struct LockMutex : ValueMapConfig<KeyT, MutexT> {<br>
>>> struct ExtraData {<br>
>>> - sys::Mutex *M;<br>
>>> + mutex_type *M;<br>
>>> bool *CalledRAUW;<br>
>>> bool *CalledDeleted;<br>
>>> };<br>
>>> @@ -192,15 +192,15 @@ struct LockMutex : ValueMapConfig<KeyT><br>
>>> *Data.CalledDeleted = true;<br>
>>> EXPECT_FALSE(Data.M->tryacquire()) << "Mutex should already be<br>
>>> locked.";<br>
>>> }<br>
>>> - static sys::Mutex *getMutex(const ExtraData &Data) { return Data.M; }<br>
>>> + static mutex_type *getMutex(const ExtraData &Data) { return Data.M; }<br>
>>> };<br>
>>> #if LLVM_ENABLE_THREADS<br>
>>> TYPED_TEST(ValueMapTest, LocksMutex) {<br>
>>> sys::Mutex M(false); // Not recursive.<br>
>>> bool CalledRAUW = false, CalledDeleted = false;<br>
>>> - typename LockMutex<TypeParam*>::ExtraData Data =<br>
>>> - {&M, &CalledRAUW, &CalledDeleted};<br>
>>> - ValueMap<TypeParam*, int, LockMutex<TypeParam*> > VM(Data);<br>
>>> + typedef LockMutex<TypeParam*, sys::Mutex> ConfigType;<br>
>>> + typename ConfigType::ExtraData Data = {&M, &CalledRAUW,<br>
>>> &CalledDeleted};<br>
>>> + ValueMap<TypeParam*, int, ConfigType> VM(Data);<br>
>>> VM[this->BitcastV.get()] = 7;<br>
>>> this->BitcastV->replaceAllUsesWith(this->AddV.get());<br>
>>> this->AddV.reset();<br>
>>><br>
>>><br>
>>> _______________________________________________<br>
>>> llvm-commits mailing list<br>
>>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
</div></div></blockquote></div><br></div>