> On Aug 17, 2016, at 2:08 PM, Zhuravlyov, Konstantin <Konstantin.Zhuravlyov at amd.com> wrote: > > >Why not going with a metadata attachment directly and kill the "singlethread" keyword? Something like: > >Something like: > > cmpxchg i32* %addr, i32 42, i32 0 monotonic monotonic, 3, !memory.scope{!42} > > cmpxchg i32* %addr, i32 42, i32 0 monotonic monotonic, 3, !memory.scope{!43} > > >... > > >!42 = !{"singlethread"} > >!43 = !{"L2"} > > >It is not clear to me if there is any correctness issues to dropping metadata? > > Yes, we cannot use the metadata approach since this metadata can be dropped during the processing of one module but not dropped in the processing of a second module, potentially resulting in inconsistent scopes for synchronizing operations leading to data races and subsequently leading to correctness issues. >Right, I saw Sameer's explanation for that earlier, and we shouldn’t move forward (without Philip’s opinion on the topic as he expressed concerns). But you stripped out the second part of my email where I wrote "It seems you’re going back to integer, which I don’t really like for reasons mentioned earlier in this thread, and that I don’t feel you addressed here”. Why can’t `synchscope` take a string literal? — Mehdi> > > From: Zhuravlyov, Konstantin > Sent: Wednesday, August 17, 2016 4:29:30 PM > To: Sameer Sahasrabuddhe; Philip Reames > Cc: Mehdi Amini; Liu, Yaxun (Sam); Ke Bai; Mekhanoshin, Stanislav; Sumner, Brian; llvm-dev at lists.llvm.org; Tye, Tony > Subject: RE: [llvm-dev] Memory scope proposal > > Hi, > > I have updated the review here: > https://reviews.llvm.org/D21723 <https://reviews.llvm.org/D21723> > > As Sameer pointed out, the motivation is: > In OpenCL 2.x, two atomic operations on the same atomic object need to have the same scope to prevent a data race. This derives from the definition of "inclusive scope" in OpenCL 2.x. Encoding OpenCL 2.x scope as metadata in LLVM IR would be a problem because there cannot be a "safe default value" to be used when the metadata is dropped. If the "largest" scope is used as the default, then the optimizer must guarantee that the metadata is dropped from every atomic operation in the whole program, or not dropped at all. > > Thanks, > Konstantin > > From: Sameer Sahasrabuddhe [mailto:sameer at sbuddhe.net] > Sent: Sunday, July 10, 2016 4:06 AM > To: Philip Reames <listmail at philipreames.com> > Cc: Mehdi Amini <mehdi.amini at apple.com>; Liu, Yaxun (Sam) <Yaxun.Liu at amd.com>; Ke Bai <kebai613 at gmail.com>; Mekhanoshin, Stanislav <Stanislav.Mekhanoshin at amd.com>; Sumner, Brian <Brian.Sumner at amd.com>; llvm-dev at lists.llvm.org; Zhuravlyov, Konstantin <Konstantin.Zhuravlyov at amd.com>; Tye, Tony <Tony.Tye at amd.com> > Subject: Re: [llvm-dev] Memory scope proposal > > > On Mon, Jul 4, 2016 at 5:09 AM, Philip Reames via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: > I will comment - as one of the few people actually working on llvm's atomic implementation with any regularity - that I am opposed to extending the instructions without a strong motivating case. I don't care anywhere near as much about metadata based schemes, but extending the instruction semantics imposes a much larger burden on the rest of the community. That burden has to be well justified and supported. > > In OpenCL 2.x, two atomic operations on the same atomic object need to have the same scope to prevent a data race. This derives from the definition of "inclusive scope" in OpenCL 2.x. Encoding OpenCL 2.x scope as metadata in LLVM IR would be a problem because there cannot be a "safe default value" to be used when the metadata is dropped. If the "largest" scope is used as the default, then the optimizer must guarantee that the metadata is dropped from every atomic operation in the whole program, or not dropped at all. > > Hence the original attempt to extend LLVM atomic instructions with a broader scope field. > > Sameer.-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160817/4ae128ce/attachment.html>
I'm coming at this from a CUDA perspective, so apologies if this doesn't make a lot of sense: In CUDA we have a similar problem as OpenCL. CUDA solves it by having a bunch of atomic builtins for each of the memory scopes. These map to various llvm target-specific intrinsics. It's not great, because the intrinsics are mostly opaque to the optimizer. But atomic ops are already pretty slow on the GPU, so I've been operating under the assumption that this isn't hurting us too much. Am I wrong about that? On Wed, Aug 17, 2016 at 3:05 PM, Mehdi Amini via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > On Aug 17, 2016, at 2:08 PM, Zhuravlyov, Konstantin > <Konstantin.Zhuravlyov at amd.com> wrote: > >>Why not going with a metadata attachment directly and kill the >> "singlethread" keyword? Something like: >>Something like: >> cmpxchg i32* %addr, i32 42, i32 0 monotonic monotonic, 3, >> !memory.scope{!42} >> cmpxchg i32* %addr, i32 42, i32 0 monotonic monotonic, 3, >> !memory.scope{!43} > >>... > >>!42 = !{"singlethread"} >>!43 = !{"L2"} > >>It is not clear to me if there is any correctness issues to dropping >> metadata? > > Yes, we cannot use the metadata approach since this metadata can be dropped > during the processing of one module but not dropped in the processing of a > second module, potentially resulting in inconsistent scopes for > synchronizing operations leading to data races and subsequently leading to > correctness issues. > > > > Right, I saw Sameer's explanation for that earlier, and we shouldn’t move > forward (without Philip’s opinion on the topic as he expressed concerns). > > But you stripped out the second part of my email where I wrote "It seems > you’re going back to integer, which I don’t really like for reasons > mentioned earlier in this thread, and that I don’t feel you addressed here”. > Why can’t `synchscope` take a string literal? > > > — > Mehdi > > > > > > > > ________________________________ > From: Zhuravlyov, Konstantin > Sent: Wednesday, August 17, 2016 4:29:30 PM > To: Sameer Sahasrabuddhe; Philip Reames > Cc: Mehdi Amini; Liu, Yaxun (Sam); Ke Bai; Mekhanoshin, Stanislav; Sumner, > Brian; llvm-dev at lists.llvm.org; Tye, Tony > Subject: RE: [llvm-dev] Memory scope proposal > > Hi, > > I have updated the review here: > https://reviews.llvm.org/D21723 > > As Sameer pointed out, the motivation is: > In OpenCL 2.x, two atomic operations on the same atomic object need to have > the same scope to prevent a data race. This derives from the definition of > "inclusive scope" in OpenCL 2.x. Encoding OpenCL 2.x scope as metadata in > LLVM IR would be a problem because there cannot be a "safe default value" to > be used when the metadata is dropped. If the "largest" scope is used as the > default, then the optimizer must guarantee that the metadata is dropped from > every atomic operation in the whole program, or not dropped at all. > > Thanks, > Konstantin > > From: Sameer Sahasrabuddhe [mailto:sameer at sbuddhe.net] > Sent: Sunday, July 10, 2016 4:06 AM > To: Philip Reames <listmail at philipreames.com> > Cc: Mehdi Amini <mehdi.amini at apple.com>; Liu, Yaxun (Sam) > <Yaxun.Liu at amd.com>; Ke Bai <kebai613 at gmail.com>; Mekhanoshin, Stanislav > <Stanislav.Mekhanoshin at amd.com>; Sumner, Brian <Brian.Sumner at amd.com>; > llvm-dev at lists.llvm.org; Zhuravlyov, Konstantin > <Konstantin.Zhuravlyov at amd.com>; Tye, Tony <Tony.Tye at amd.com> > Subject: Re: [llvm-dev] Memory scope proposal > > > On Mon, Jul 4, 2016 at 5:09 AM, Philip Reames via llvm-dev > <llvm-dev at lists.llvm.org> wrote: > > I will comment - as one of the few people actually working on llvm's atomic > implementation with any regularity - that I am opposed to extending the > instructions without a strong motivating case. I don't care anywhere near > as much about metadata based schemes, but extending the instruction > semantics imposes a much larger burden on the rest of the community. That > burden has to be well justified and supported. > > > In OpenCL 2.x, two atomic operations on the same atomic object need to have > the same scope to prevent a data race. This derives from the definition of > "inclusive scope" in OpenCL 2.x. Encoding OpenCL 2.x scope as metadata in > LLVM IR would be a problem because there cannot be a "safe default value" to > be used when the metadata is dropped. If the "largest" scope is used as the > default, then the optimizer must guarantee that the metadata is dropped from > every atomic operation in the whole program, or not dropped at all. > > Hence the original attempt to extend LLVM atomic instructions with a broader > scope field. > > Sameer. > > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >
On 08/17/2016 03:05 PM, Mehdi Amini wrote:> >> On Aug 17, 2016, at 2:08 PM, Zhuravlyov, Konstantin >> <Konstantin.Zhuravlyov at amd.com >> <mailto:Konstantin.Zhuravlyov at amd.com>> wrote: >> >> >Why not going with a metadata attachment directly and kill the "singlethread" keyword? >> Something like: >> >Something like: >> > cmpxchg i32* %addr, i32 42, i32 0 monotonic monotonic, 3, >> !memory.scope{!42} >> > cmpxchg i32* %addr, i32 42, i32 0 monotonic monotonic, 3, >> !memory.scope{!43} >> >... >> >!42 = !{"singlethread"} >> >!43 = !{"L2"} >> >It is not clear to me if there is any correctness issues to dropping >> metadata? >> Yes, we cannot use the metadata approach since this metadata can be >> dropped during the processing of one module but not dropped in the >> processing of a second module, potentially resulting in inconsistent >> scopes for synchronizing operations leading to data races and >> subsequently leading to correctness issues. > > Right, I saw Sameer's explanation for that earlier, and we shouldn’t > move forward (without Philip’s opinion on the topic as he expressed > concerns).Given my current time commitments, having me on the critical path for any proposal is not a good idea. I'm willing to step aside here as long as the proposal is well reviewed by someone who's familiar with the memory model. Hal, Sanjoy, JF, Chandler, and Danny would all be reasonable alternates. Mehdi, if things get to the point where you think they're good to go and no one else has chimed in, ping me. I'm not going to be following until then, but I'll make the time for a final pass through if no one else has first.> > But you stripped out the second part of my email where I wrote "It > seems you’re going back to integer, which I don’t really like for > reasons mentioned earlier in this thread, and that I don’t feel you > addressed here”. Why can’t `synchscope` take a string literal? > > > — > Mehdi > > > > > >> >> ------------------------------------------------------------------------ >> *From:*Zhuravlyov, Konstantin >> *Sent:*Wednesday, August 17, 2016 4:29:30 PM >> *To:*Sameer Sahasrabuddhe; Philip Reames >> *Cc:*Mehdi Amini; Liu, Yaxun (Sam); Ke Bai; Mekhanoshin, Stanislav; >> Sumner, Brian; llvm-dev at lists.llvm.org >> <mailto:llvm-dev at lists.llvm.org>; Tye, Tony >> *Subject:*RE: [llvm-dev] Memory scope proposal >> Hi, >> I have updated the review here: >> https://reviews.llvm.org/D21723 >> As Sameer pointed out, the motivation is: >> In OpenCL 2.x, two atomic operations on the same atomic object need >> to have the same scope to prevent a data race. This derives from the >> definition of "inclusive scope" in OpenCL 2.x. Encoding OpenCL 2.x >> scope as metadata in LLVM IR would be a problem because there cannot >> be a "safe default value" to be used when the metadata is dropped. If >> the "largest" scope is used as the default, then the optimizer must >> guarantee that the metadata is dropped from every atomic operation in >> the whole program, or not dropped at all. >> Thanks, >> Konstantin >> *From:*Sameer Sahasrabuddhe [mailto:sameer at sbuddhe.net] >> *Sent:*Sunday, July 10, 2016 4:06 AM >> *To:*Philip Reames <listmail at philipreames.com >> <mailto:listmail at philipreames.com>> >> *Cc:*Mehdi Amini <mehdi.amini at apple.com >> <mailto:mehdi.amini at apple.com>>; Liu, Yaxun (Sam) <Yaxun.Liu at amd.com >> <mailto:Yaxun.Liu at amd.com>>; Ke Bai <kebai613 at gmail.com >> <mailto:kebai613 at gmail.com>>; Mekhanoshin, Stanislav >> <Stanislav.Mekhanoshin at amd.com >> <mailto:Stanislav.Mekhanoshin at amd.com>>; Sumner, Brian >> <Brian.Sumner at amd.com <mailto:Brian.Sumner at amd.com>>; >> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>; Zhuravlyov, >> Konstantin <Konstantin.Zhuravlyov at amd.com >> <mailto:Konstantin.Zhuravlyov at amd.com>>; Tye, Tony <Tony.Tye at amd.com >> <mailto:Tony.Tye at amd.com>> >> *Subject:*Re: [llvm-dev] Memory scope proposal >> On Mon, Jul 4, 2016 at 5:09 AM, Philip Reames via llvm-dev >> <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: >> >> I will comment - as one of the few people actually working on >> llvm's atomic implementation with any regularity - that I am >> opposed to extending the instructions without a strong motivating >> case. I don't care anywhere near as much about metadata based >> schemes, but extending the instruction semantics imposes a much >> larger burden on the rest of the community. That burden has to >> be well justified and supported. >> >> >> In OpenCL 2.x, two atomic operations on the same atomic object need >> to have the same scope to prevent a data race. This derives from the >> definition of "inclusive scope" in OpenCL 2.x. Encoding OpenCL 2.x >> scope as metadata in LLVM IR would be a problem because there cannot >> be a "safe default value" to be used when the metadata is dropped. If >> the "largest" scope is used as the default, then the optimizer must >> guarantee that the metadata is dropped from every atomic operation in >> the whole program, or not dropped at all. >> >> Hence the original attempt to extend LLVM atomic instructions with a >> broader scope field. >> >> Sameer. >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160821/bbebf462/attachment.html>
> On Aug 21, 2016, at 11:14 AM, Philip Reames <listmail at philipreames.com> wrote: > > On 08/17/2016 03:05 PM, Mehdi Amini wrote: >> >>> On Aug 17, 2016, at 2:08 PM, Zhuravlyov, Konstantin <Konstantin.Zhuravlyov at amd.com <mailto:Konstantin.Zhuravlyov at amd.com>> wrote: >>> >>> >Why not going with a metadata attachment directly and kill the "singlethread" keyword? Something like: >>> >Something like: >>> > cmpxchg i32* %addr, i32 42, i32 0 monotonic monotonic, 3, !memory.scope{!42} >>> > cmpxchg i32* %addr, i32 42, i32 0 monotonic monotonic, 3, !memory.scope{!43} >>> >>> >... >>> >>> >!42 = !{"singlethread"} >>> >!43 = !{"L2"} >>> >>> >It is not clear to me if there is any correctness issues to dropping metadata? >>> >>> Yes, we cannot use the metadata approach since this metadata can be dropped during the processing of one module but not dropped in the processing of a second module, potentially resulting in inconsistent scopes for synchronizing operations leading to data races and subsequently leading to correctness issues. >>> >> >> Right, I saw Sameer's explanation for that earlier, and we shouldn’t move forward (without Philip’s opinion on the topic as he expressed concerns). > Given my current time commitments, having me on the critical path for any proposal is not a good idea. I'm willing to step aside here as long as the proposal is well reviewed by someone who's familiar with the memory model. Hal, Sanjoy, JF, Chandler, and Danny would all be reasonable alternates.OK, good to know. I put you on the path because you wrote: "I am opposed to extending the instructions without a strong motivating case. I don't care anywhere near as much about metadata based schemes, but extending the instruction semantics imposes a much larger burden on the rest of the community. That burden has to be well justified and supported." It is not clear to me right now if the "use case" makes it "well justified" or not (an alternative being using intrinsic for OpenCL as Justin Lebar mentioned). I don’t feel I can answer this, so adding CC Chandler and Sanjoy to begin with. — Mehdi> Mehdi, if things get to the point where you think they're good to go and no one else has chimed in, ping me. I'm not going to be following until then, but I'll make the time for a final pass through if no one else has first. >> >> But you stripped out the second part of my email where I wrote "It seems you’re going back to integer, which I don’t really like for reasons mentioned earlier in this thread, and that I don’t feel you addressed here”. Why can’t `synchscope` take a string literal? >> >> >> — >> Mehdi >> >> >> >> >> >>> >>> >>> From: Zhuravlyov, Konstantin >>> Sent: Wednesday, August 17, 2016 4:29:30 PM >>> To: Sameer Sahasrabuddhe; Philip Reames >>> Cc: Mehdi Amini; Liu, Yaxun (Sam); Ke Bai; Mekhanoshin, Stanislav; Sumner, Brian; llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>; Tye, Tony >>> Subject: RE: [llvm-dev] Memory scope proposal >>> >>> Hi, >>> >>> I have updated the review here: >>> https://reviews.llvm.org/D21723 <https://reviews.llvm.org/D21723> >>> >>> As Sameer pointed out, the motivation is: >>> In OpenCL 2.x, two atomic operations on the same atomic object need to have the same scope to prevent a data race. This derives from the definition of "inclusive scope" in OpenCL 2.x. Encoding OpenCL 2.x scope as metadata in LLVM IR would be a problem because there cannot be a "safe default value" to be used when the metadata is dropped. If the "largest" scope is used as the default, then the optimizer must guarantee that the metadata is dropped from every atomic operation in the whole program, or not dropped at all. >>> >>> Thanks, >>> Konstantin >>> >>> From: Sameer Sahasrabuddhe [mailto:sameer at sbuddhe.net <mailto:sameer at sbuddhe.net>] >>> Sent: Sunday, July 10, 2016 4:06 AM >>> To: Philip Reames <listmail at philipreames.com <mailto:listmail at philipreames.com>> >>> Cc: Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>>; Liu, Yaxun (Sam) <Yaxun.Liu at amd.com <mailto:Yaxun.Liu at amd.com>>; Ke Bai <kebai613 at gmail.com <mailto:kebai613 at gmail.com>>; Mekhanoshin, Stanislav <Stanislav.Mekhanoshin at amd.com <mailto:Stanislav.Mekhanoshin at amd.com>>; Sumner, Brian <Brian.Sumner at amd.com <mailto:Brian.Sumner at amd.com>>; llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>; Zhuravlyov, Konstantin <Konstantin.Zhuravlyov at amd.com <mailto:Konstantin.Zhuravlyov at amd.com>>; Tye, Tony <Tony.Tye at amd.com <mailto:Tony.Tye at amd.com>> >>> Subject: Re: [llvm-dev] Memory scope proposal >>> >>> >>> On Mon, Jul 4, 2016 at 5:09 AM, Philip Reames via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: >>> I will comment - as one of the few people actually working on llvm's atomic implementation with any regularity - that I am opposed to extending the instructions without a strong motivating case. I don't care anywhere near as much about metadata based schemes, but extending the instruction semantics imposes a much larger burden on the rest of the community. That burden has to be well justified and supported. >>> >>> In OpenCL 2.x, two atomic operations on the same atomic object need to have the same scope to prevent a data race. This derives from the definition of "inclusive scope" in OpenCL 2.x. Encoding OpenCL 2.x scope as metadata in LLVM IR would be a problem because there cannot be a "safe default value" to be used when the metadata is dropped. If the "largest" scope is used as the default, then the optimizer must guarantee that the metadata is dropped from every atomic operation in the whole program, or not dropped at all. >>> >>> Hence the original attempt to extend LLVM atomic instructions with a broader scope field. >>> >>> Sameer. >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160821/9e02f225/attachment.html>