<div dir="ltr">The motivation is just that we should be relying on standard implementations as they're more likely to be robust, battle tested, and higher performance than our own hand-rolled implementations. For example, there are many locations in the code that roll a custom version of once-only initialization. As a future task, I would like to convert all of them to std::call_once. Just as a general principle of not re-inventing the wheel.<div>
<br></div><div>As for the platform specific header includes, I guess that's ultimately a separate discussion, and this change can be made without introducing platform-specific header includes into Mutex.h. I am currently working on a revision to this patch which removes the windows.h include from mutex.h, so I will upload that later.</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jun 6, 2014 at 12:32 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Zachary,<br>
<br>
In LLVM platform-specific code goes in the implementation files (.inc) and never in the portable headers. Certainly for a high-level class like Mutex I wouldn't expect to see platform-specific conditionals exposed.<br>
<br>
A couple of exceptions have slipped in like Support/FileSystem.h and Support/Process.h with ifdef WIN32 but those are meant to get fixed at some point.<br>
<br>
On a more practical level it's not a good idea to include windows.h globally throughout the public headers. These headers need to get included in portable software that embeds clang for tooling etc. and windows.h is known to be problematic.<br>
<br>
For thread primitives, the previous implementation looks broadly correct. You've explained the mechanics of your changes in detail but can you explain the motivation here? We need to find a better way to achieve that without introducing all these OS-specific low-level headers globally into embedding applications.<br>
<br>
Alp.<div><div class="h5"><br>
<br>
<br>
<br>
On 05/06/2014 20:06, Zachary Turner wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
Hi rnk, aaron.ballman, chandlerc,<br>
<br>
This is a re-submission of an earlier broken patch.<br>
<br>
Change llvm::sys::Mutex implementation to use std::mutex and std::recursive_mutex.<br>
<br>
This patch deletes the platform specific mutex implementations, and delegates to the underlying STL implementation. Because STL mutex and recursive_mutex are implemented as separate classes, it is no longer possible to specify as a runtime param whether a recursive mutex is desired, and instead the decision must be made at compile time.<br>
<br>
As part of this refactor, the ScopedLock class is deleted in favor of MutexGuard, which provides equivalent functionality.<br>
<br>
To make reviewing easier, you can refer to the following table of equivalences in the format (old_type -> new_type)<br>
<br>
SmartMutex<true>([true]) -> RecursiveDebugMutex<br>
<br>
SmartMutex<false>([true]) -> RecursiveMutex<br>
sys::Mutex -> SmartMutex<false>() -> RecursiveMutex<br>
<br>
SmartMutex<true>(false) -> NonRecursiveDebugMutex<br>
SmartMutex<false>(false) -> NonRecursiveMutex<br>
<br>
One caveat of note is that on Windows std::*mutex cannot be used during an atexit handler. To get around this, a new type AtexitSafeMutex is defined, and on Windows this is typedefed to a critical section.<br>
<br>
<a href="http://reviews.llvm.org/D4033" target="_blank">http://reviews.llvm.org/D4033</a><br>
<br>
Files:<br>
include/llvm/ExecutionEngine/<u></u>ExecutionEngine.h<br>
include/llvm/IR/ValueMap.h<br>
include/llvm/Support/Mutex.h<br>
include/llvm/Support/<u></u>MutexGuard.h<br>
lib/CodeGen/PseudoSourceValue.<u></u>cpp<br>
lib/CodeGen/SelectionDAG/<u></u>SelectionDAG.cpp<br>
lib/ExecutionEngine/<u></u>ExecutionEngine.cpp<br>
lib/ExecutionEngine/<u></u>Interpreter/ExternalFunctions.<u></u>cpp<br>
lib/ExecutionEngine/JIT/JIT.<u></u>cpp<br>
lib/ExecutionEngine/JIT/<u></u>JITEmitter.cpp<br>
lib/ExecutionEngine/<u></u>OProfileJIT/OProfileWrapper.<u></u>cpp<br>
lib/ExecutionEngine/<u></u>RuntimeDyld/GDBRegistrar.cpp<br>
lib/ExecutionEngine/<u></u>RuntimeDyld/RuntimeDyldImpl.h<br>
lib/IR/LeakDetector.cpp<br>
lib/IR/LegacyPassManager.cpp<br>
lib/Support/CMakeLists.txt<br>
lib/Support/<u></u>CrashRecoveryContext.cpp<br>
lib/Support/DynamicLibrary.cpp<br>
lib/Support/Mutex.cpp<br>
lib/Support/PluginLoader.cpp<br>
lib/Support/Statistic.cpp<br>
lib/Support/Threading.cpp<br>
lib/Support/Timer.cpp<br>
lib/Support/Unix/Mutex.inc<br>
lib/Support/Unix/Process.inc<br>
lib/Support/Unix/Signals.inc<br>
lib/Support/Windows/<u></u>DynamicLibrary.inc<br>
lib/Support/Windows/Mutex.inc<br>
lib/Target/NVPTX/<u></u>NVPTXUtilities.cpp<br>
unittests/IR/ValueMapTest.cpp<br>
<br>
<br></div></div>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">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/<u></u>mailman/listinfo/llvm-commits</a><span class="HOEnZb"><font color="#888888"><br>
</font></span></blockquote><span class="HOEnZb"><font color="#888888">
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</font></span></blockquote></div><br></div>