<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Thanks for your comment!<div class="">I updated the patch.</div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 20 Feb 2015, at 17:05, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><p dir="ltr" class=""><br class="">
On Feb 20, 2015 5:28 AM, "Erik Eckstein" <<a href="mailto:eeckstein@apple.com" class="">eeckstein@apple.com</a>> wrote:<br class="">
><br class="">
> With this patch I'm trying to solve the following problem:<br class="">
><br class="">
> I'm running llvm in multiple threads (each instance uses its private LLVMContext).<br class="">
> With a release-asserts build of llvm I got terrible multi-thread performance. I found that the problem is the mutex in PassRegistry::getPassInfo(), mainly called from PMDataManager::verifyPreservedAnalysis().<br class="">
><br class="">
> My first idea was to make an option to disable verifyPreservedAnalysis(), even in an asserts-build.<br class="">
> But I didn't like it because I don't want to give up this verification and the mutex also cause overhead outside verifyPreservedAnalysis().<br class="">
><br class="">
> So I did the following:<br class="">
> I added a lock() function in PassRegistry which I call when I'm sure that all passes are registered.<br class="">
> After the PassRegistry is locked it can safely access its maps without using a mutex.<br class="">
><br class="">
> This completely solves the multi-thread performance problem.<br class="">
> The use of the lock() function is optional. If not used, nothing changes.<br class="">
><br class="">
> <a href="http://reviews.llvm.org/D7787" class="">http://reviews.llvm.org/D7787</a><br class="">
><br class="">
> Files:<br class="">
> include/llvm/PassRegistry.h<br class="">
> lib/IR/PassRegistry.cpp<br class="">
><br class="">
> Index: include/llvm/PassRegistry.h<br class="">
> ===================================================================<br class="">
> --- include/llvm/PassRegistry.h<br class="">
> +++ include/llvm/PassRegistry.h<br class="">
> @@ -41,6 +41,9 @@<br class="">
> class PassRegistry {<br class="">
> mutable sys::SmartRWMutex<true> Lock;<br class="">
><br class="">
> + /// Only if false, synchronization must use the Lock mutex.<br class="">
> + std::atomic<bool> locked;<br class="">
> +<br class="">
> /// PassInfoMap - Keep track of the PassInfo object for each registered pass.<br class="">
> typedef DenseMap<const void *, const PassInfo *> MapType;<br class="">
> MapType PassInfoMap;<br class="">
> @@ -52,7 +55,7 @@<br class="">
> std::vector<PassRegistrationListener *> Listeners;<br class="">
><br class="">
> public:<br class="">
> - PassRegistry() {}<br class="">
> + PassRegistry() : locked(false) {}<br class="">
> ~PassRegistry();<br class="">
><br class="">
> /// getPassRegistry - Access the global registry object, which is<br class="">
> @@ -60,6 +63,10 @@<br class="">
> /// llvm_shutdown.<br class="">
> static PassRegistry *getPassRegistry();<br class="">
><br class="">
> + /// Enables fast thread synchronization in getPassInfo().<br class="">
> + /// After calling lock() no more passes may be registered.<br class="">
> + void lock() { locked = true; }<br class="">
> +<br class="">
> /// getPassInfo - Look up a pass' corresponding PassInfo, indexed by the pass'<br class="">
> /// type identifier (&MyPass::ID).<br class="">
> const PassInfo *getPassInfo(const void *TI) const;<br class="">
> Index: lib/IR/PassRegistry.cpp<br class="">
> ===================================================================<br class="">
> --- lib/IR/PassRegistry.cpp<br class="">
> +++ lib/IR/PassRegistry.cpp<br class="">
> @@ -39,15 +39,29 @@<br class="">
> PassRegistry::~PassRegistry() {}<br class="">
><br class="">
> const PassInfo *PassRegistry::getPassInfo(const void *TI) const {<br class="">
> - sys::SmartScopedReader<true> Guard(Lock);<br class="">
> - MapType::const_iterator I = PassInfoMap.find(TI);<br class="">
> - return I != PassInfoMap.end() ? I->second : nullptr;<br class="">
> + if (locked) {<br class="">
> + // We don't need thread synchronization because the PassInfoMap is read-only<br class="">
> + // after the PassRegistry is locked.<br class="">
> + MapType::const_iterator I = PassInfoMap.find(TI);<br class="">
> + return I != PassInfoMap.end() ? I->second : nullptr;<br class="">
> + } else {</p><p dir="ltr" class="">Else after return (& another way Tod o this might be to put the guard in an Optional object and then just share the actual lookup code)</p><p dir="ltr" class="">> + sys::SmartScopedReader<true> Guard(Lock);<br class="">
> + MapType::const_iterator I = PassInfoMap.find(TI);<br class="">
> + return I != PassInfoMap.end() ? I->second : nullptr;<br class="">
> + }<br class="">
> }<br class="">
><br class="">
> const PassInfo *PassRegistry::getPassInfo(StringRef Arg) const {<br class="">
> - sys::SmartScopedReader<true> Guard(Lock);<br class="">
> - StringMapType::const_iterator I = PassInfoStringMap.find(Arg);<br class="">
> - return I != PassInfoStringMap.end() ? I->second : nullptr;<br class="">
> + if (locked) {<br class="">
> + // We don't need thread synchronization because the PassInfoStringMap is<br class="">
> + // read-only after the PassRegistry is locked.<br class="">
> + StringMapType::const_iterator I = PassInfoStringMap.find(Arg);<br class="">
> + return I != PassInfoStringMap.end() ? I->second : nullptr;<br class="">
> + } else {<br class="">
> + sys::SmartScopedReader<true> Guard(Lock);<br class="">
> + StringMapType::const_iterator I = PassInfoStringMap.find(Arg);<br class="">
> + return I != PassInfoStringMap.end() ? I->second : nullptr;<br class="">
> + }<br class="">
> }<br class="">
><br class="">
> //===----------------------------------------------------------------------===//<br class="">
> @@ -55,6 +69,9 @@<br class="">
> //<br class="">
><br class="">
> void PassRegistry::registerPass(const PassInfo &PI, bool ShouldFree) {<br class="">
> +<br class="">
> + assert(!locked && "Trying to register a pass in a locked PassRegistry");<br class="">
> +<br class="">
> sys::SmartScopedWriter<true> Guard(Lock);<br class="">
> bool Inserted =<br class="">
> PassInfoMap.insert(std::make_pair(PI.getTypeInfo(), &PI)).second;<br class="">
> @@ -68,6 +85,8 @@<br class="">
><br class="">
> if (ShouldFree)<br class="">
> ToFree.push_back(std::unique_ptr<const PassInfo>(&PI));<br class="">
> +<br class="">
> + assert(!locked && "PassRegistry locked during registering a pass");<br class="">
> }<br class="">
><br class="">
> void PassRegistry::enumerateWith(PassRegistrationListener *L) {<br class="">
><br class="">
> EMAIL PREFERENCES<br class="">
> <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" class="">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br class="">
><br class="">
> _______________________________________________<br class="">
> llvm-commits mailing list<br class="">
> <a href="mailto:llvm-commits@cs.uiuc.edu" class="">llvm-commits@cs.uiuc.edu</a><br class="">
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br class="">
><br class="">
</p>
</div></blockquote></div><br class=""></div></body></html>