<div dir="ltr">Backing up a second, and setting aside all aspects of windows.h, I think this patch is going in the wrong direction at a very fundamental level.<div><br></div><div>I think it is a really huge mistake for LLVM to continue to use its own Mutex class. I don't think you should change it, I think you should remove it, and use std::mutex. I understand that this may be hard, but I think time would be better spent working on those hard problems. I see a few elements that you'll need to address:</div>
<div><br></div><div>1) Fixing the code in atexit handlers. I'm not 100% certain of the right approach here, but my strong suspicion is that they should not be using a mutex *at all*. LLVM should have *very* little code in atexit handlers as a library, and that code really should be customized for the single threaded and strange circumstance in which it runs. I know you've already started down this path. In general, that should be a nice separable bit.</div>
<div><br></div><div>2) You or someone else should start a discussion on llvmdev@ about whether or not we need to avoid the *overhead* of an uncontended mutex lock when LLVM is not multithreaded. I think that the answer is "no" based on a lot of experience with reasonably good mutex implementations, but we need to collect more opinions about this and understand the specific use cases. If there are mutexes which materially impact the performance of LLVM when running in a single thread, they will also be performance problems for multithreaded code (by virtue of crazy amounts of contention) and should be fixed directly.</div>
<div><br></div><div>3) Perhaps even before doing #2, we should sort out whether the multithreaded support in LLVM should be a compile time or runtime parameter. While I strongly feel it should be compile time if present at all, this isn't a discussion for a long, twisty patch review thread. =] It needs its own discussion and a wider audience.</div>
<div><br></div><div><br></div><div>I also suspect you could go ahead and convert some users of mutexes to std::mutex while these things are in-flight. For example, I agree with Greg who said on the LLDB list that he would prefer LLDB just use std::mutex -- why not go ahead and start that migration? If LLD still uses something else, that would be another good candidate. Both of those have a decent history of being multithreaded always, and so it seems uncontentious there even while we try to sort out exactly what is right for the core libraries that support JITs and other more complex users.</div>
<div><br></div><div>-Chandler</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jun 6, 2014 at 4:34 PM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Remove the Mutex.h dependency on windows.h, and address various other review comments.<br>
<br>
This solution is still less than ideal, because we need to include some windows preprocessor checks inside of Mutex.h. The best solution in my opinion would require allowing some platform specific headers to be in the include tree (for example, include\llvm\Support\Windows\CriticalSectionMutex.h). This would remove all of the preprocessor code from Mutex.h, and allow anyone who needs this to simply write #include "llvm/Support/Windows/CriticalSectionMutex.h".<br>
<div class="HOEnZb"><div class="h5"><br>
<a href="http://reviews.llvm.org/D4033" target="_blank">http://reviews.llvm.org/D4033</a><br>
<br>
Files:<br>
include/llvm/ExecutionEngine/ExecutionEngine.h<br>
include/llvm/IR/ValueMap.h<br>
include/llvm/Support/Mutex.h<br>
include/llvm/Support/MutexGuard.h<br>
lib/CodeGen/PseudoSourceValue.cpp<br>
lib/CodeGen/SelectionDAG/SelectionDAG.cpp<br>
lib/ExecutionEngine/ExecutionEngine.cpp<br>
lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp<br>
lib/ExecutionEngine/JIT/JIT.cpp<br>
lib/ExecutionEngine/JIT/JITEmitter.cpp<br>
lib/ExecutionEngine/OProfileJIT/OProfileWrapper.cpp<br>
lib/ExecutionEngine/RuntimeDyld/GDBRegistrar.cpp<br>
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h<br>
lib/IR/LeakDetector.cpp<br>
lib/IR/LegacyPassManager.cpp<br>
lib/Support/CMakeLists.txt<br>
lib/Support/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/DynamicLibrary.inc<br>
lib/Support/Windows/Mutex.inc<br>
lib/Target/NVPTX/NVPTXUtilities.cpp<br>
unittests/IR/ValueMapTest.cpp<br>
</div></div><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></blockquote></div><br></div>