[PATCH] Change base mutex implementations to use STL-provided mutexes

Chandler Carruth chandlerc at gmail.com
Fri Jun 6 18:06:41 PDT 2014


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.

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:

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.

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.

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.


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.

-Chandler


On Fri, Jun 6, 2014 at 4:34 PM, Zachary Turner <zturner at google.com> wrote:

> Remove the Mutex.h dependency on windows.h, and address various other
> review comments.
>
> 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".
>
> http://reviews.llvm.org/D4033
>
> Files:
>   include/llvm/ExecutionEngine/ExecutionEngine.h
>   include/llvm/IR/ValueMap.h
>   include/llvm/Support/Mutex.h
>   include/llvm/Support/MutexGuard.h
>   lib/CodeGen/PseudoSourceValue.cpp
>   lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>   lib/ExecutionEngine/ExecutionEngine.cpp
>   lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp
>   lib/ExecutionEngine/JIT/JIT.cpp
>   lib/ExecutionEngine/JIT/JITEmitter.cpp
>   lib/ExecutionEngine/OProfileJIT/OProfileWrapper.cpp
>   lib/ExecutionEngine/RuntimeDyld/GDBRegistrar.cpp
>   lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h
>   lib/IR/LeakDetector.cpp
>   lib/IR/LegacyPassManager.cpp
>   lib/Support/CMakeLists.txt
>   lib/Support/CrashRecoveryContext.cpp
>   lib/Support/DynamicLibrary.cpp
>   lib/Support/Mutex.cpp
>   lib/Support/PluginLoader.cpp
>   lib/Support/Statistic.cpp
>   lib/Support/Threading.cpp
>   lib/Support/Timer.cpp
>   lib/Support/Unix/Mutex.inc
>   lib/Support/Unix/Process.inc
>   lib/Support/Unix/Signals.inc
>   lib/Support/Windows/DynamicLibrary.inc
>   lib/Support/Windows/Mutex.inc
>   lib/Target/NVPTX/NVPTXUtilities.cpp
>   unittests/IR/ValueMapTest.cpp
>
> _______________________________________________
> 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/20140606/5a549030/attachment.html>


More information about the llvm-commits mailing list