On Apr 12, 2020, at 2:23 PM, Eli Friedman <efriedma at quicinc.com> wrote:> > Yes, the llvm::Smart* family of locks still exist. But very few places are using them outside of MLIR; it’s more common to just use plain std::mutex. > > That said, I don’t think it’s really a good idea to use them, even if they were fixed to work as designed. It’s not composable: the boolean “enabled” bit is process-wide, not local to whatever data structure you’re trying to build. So your single-threaded tool gets some benefit, but the benefit goes away as soon as the process starts using multiple threads, even if there still only one thread using the MLIR context in question.Yes, I agree, this is similar to Mehdi’s point. I think it is clear that “enable” and “disable” multithreaded mode should only be called from applications, not libraries. Calling one of them from a library breaks composability.> So probably I’d recommend two things: > If locking uncontended locks is showing up on profiles as a performance bottleneck, it’s probably worth looking into ways to reduce that overhead in both single-threaded and multi-threaded contexts. (Reducing the number of locks taken in frequently called code, or using a better lock implementation). > If you want some mechanism to disable MLIR locking, it should probably be a boolean attached to the MLIR context in question, not a global variable. >Ok, but let me argue the other way. We currently have a cmake flag that sets LLVM_ENABLE_THREADS, and that flag enables an across the board speedup. That cmake flag is the *worst* possible thing for library composability. :-). Are you suggesting that we remove it? -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200412/1541072a/attachment.html>
(reply inline) From: Chris Lattner <clattner at nondot.org> Sent: Sunday, April 12, 2020 3:28 PM To: Eli Friedman <efriedma at quicinc.com> Cc: llvm-dev <llvm-dev at lists.llvm.org> Subject: [EXT] Re: [llvm-dev] LLVM multithreading support On Apr 12, 2020, at 2:23 PM, Eli Friedman <efriedma at quicinc.com<mailto:efriedma at quicinc.com>> wrote: So probably I’d recommend two things: 1. If locking uncontended locks is showing up on profiles as a performance bottleneck, it’s probably worth looking into ways to reduce that overhead in both single-threaded and multi-threaded contexts. (Reducing the number of locks taken in frequently called code, or using a better lock implementation). 2. If you want some mechanism to disable MLIR locking, it should probably be a boolean attached to the MLIR context in question, not a global variable. Ok, but let me argue the other way. We currently have a cmake flag that sets LLVM_ENABLE_THREADS, and that flag enables an across the board speedup. That cmake flag is the *worst* possible thing for library composability. :-). Are you suggesting that we remove it? Yes, I would like to remove LLVM_ENABLE_THREADS. Assuming you’re not building for some exotic target that doesn’t have threads, there isn’t any reason to randomly shut off all thread-related functionality in the LLVM support libraries. There isn’t any significant performance or codesize gain to be had outside of MLIR, as far as I know, and it increases the number of configurations we have to worry about. I have no idea if turning it off even works on master; I don’t know of any buildbots or users using that configuration. If you want to support some sort of lockless mode in MLIR, I think that burden should be carried as part of MLIR, instead of infecting the entire LLVM codebase. -Eli -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200412/9a95d19c/attachment.html>
On Sun, Apr 12, 2020 at 3:56 PM Eli Friedman via llvm-dev < llvm-dev at lists.llvm.org> wrote:> (reply inline) > > > > *From:* Chris Lattner <clattner at nondot.org> > *Sent:* Sunday, April 12, 2020 3:28 PM > *To:* Eli Friedman <efriedma at quicinc.com> > *Cc:* llvm-dev <llvm-dev at lists.llvm.org> > *Subject:* [EXT] Re: [llvm-dev] LLVM multithreading support > > > > On Apr 12, 2020, at 2:23 PM, Eli Friedman <efriedma at quicinc.com> wrote: > > So probably I’d recommend two things: > > 1. If locking uncontended locks is showing up on profiles as a > performance bottleneck, it’s probably worth looking into ways to reduce > that overhead in both single-threaded and multi-threaded contexts. > (Reducing the number of locks taken in frequently called code, or using a > better lock implementation). > > 2. If you want some mechanism to disable MLIR locking, it should > probably be a boolean attached to the MLIR context in question, not a > global variable. > > > > > > Ok, but let me argue the other way. We currently have a cmake flag that > sets LLVM_ENABLE_THREADS, and that flag enables an across the board > speedup. That cmake flag is the *worst* possible thing for library > composability. :-). Are you suggesting that we remove it? > > > > Yes, I would like to remove LLVM_ENABLE_THREADS. Assuming you’re not > building for some exotic target that doesn’t have threads, there isn’t any > reason to randomly shut off all thread-related functionality in the LLVM > support libraries. There isn’t any significant performance or codesize > gain to be had outside of MLIR, as far as I know, and it increases the > number of configurations we have to worry about. I have no idea if turning > it off even works on master; I don’t know of any buildbots or users using > that configuration. > > > > If you want to support some sort of lockless mode in MLIR, I think that > burden should be carried as part of MLIR, instead of infecting the entire > LLVM codebase. >I agree here. It is going to be much easier, and likely saner, to have the MLIR bits controlled by a flag in the MLIR context. Trying to stop/start threading bits in utilities doesn't seem reliable given all of the different factors, e.g. ThreadPool /llvm::parallel_for/etc. functionality. It also provides a much more controlled interface/contract. -- River> -Eli > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200412/ba49bdee/attachment.html>
On Sun, Apr 12, 2020 at 3:56 PM Eli Friedman via llvm-dev < llvm-dev at lists.llvm.org> wrote:> (reply inline) > > > > *From:* Chris Lattner <clattner at nondot.org> > *Sent:* Sunday, April 12, 2020 3:28 PM > *To:* Eli Friedman <efriedma at quicinc.com> > *Cc:* llvm-dev <llvm-dev at lists.llvm.org> > *Subject:* [EXT] Re: [llvm-dev] LLVM multithreading support > > > > On Apr 12, 2020, at 2:23 PM, Eli Friedman <efriedma at quicinc.com> wrote: > > So probably I’d recommend two things: > > 1. If locking uncontended locks is showing up on profiles as a > performance bottleneck, it’s probably worth looking into ways to reduce > that overhead in both single-threaded and multi-threaded contexts. > (Reducing the number of locks taken in frequently called code, or using a > better lock implementation). > > 2. If you want some mechanism to disable MLIR locking, it should > probably be a boolean attached to the MLIR context in question, not a > global variable. > > > > > > Ok, but let me argue the other way. We currently have a cmake flag that > sets LLVM_ENABLE_THREADS, and that flag enables an across the board > speedup. That cmake flag is the *worst* possible thing for library > composability. :-). Are you suggesting that we remove it? > > > > Yes, I would like to remove LLVM_ENABLE_THREADS. Assuming you’re not > building for some exotic target that doesn’t have threads, there isn’t any > reason to randomly shut off all thread-related functionality in the LLVM > support libraries. >There isn’t any significant performance or codesize gain to be had outside> of MLIR, as far as I know, and it increases the number of configurations we > have to worry about. >Well: if one can measure performance / binary size, I think it is a valid configuration to have. Being able to build and embed the compiler without paying the price for what you don't need seems valuable to me. Of course if we're confident that there is no use (no way to use LLVM as a library and measure a difference there), removing it seems OK to me.>> I have no idea if turning it off even works on master; I don’t know of > any buildbots or users using that configuration. > > > > If you want to support some sort of lockless mode in MLIR, I think that > burden should be carried as part of MLIR, instead of infecting the entire > LLVM codebase. >I agree that we should look into improving the situation on the MLIRContext itself to reduce the cost when used outside of a multi-threaded context. It isn't great in general to maintain a modal behavior in order to dynamically manage the thread safety aspect of an object, so this will likely require quite some thoughts. -- Mehdi -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200412/91c8ef7b/attachment.html>