Atmn Patel via llvm-dev
2020-Sep-11 18:33 UTC
[llvm-dev] [RFC] Introducing the maynotprogress IR attribute
Hi James, On Fri, Sep 11, 2020 at 2:05 PM James Y Knight <jyknight at google.com> wrote:> > On Fri, Sep 11, 2020 at 12:42 PM Atmn Patel <atmndp at gmail.com> wrote: >> >> Hi Hal, >> >> On Thu, Sep 10, 2020 at 8:54 PM Hal Finkel <hfinkel at anl.gov> wrote: >> > >> > Hi, Atmn, >> > >> > Has anyone else expressed an opinion regarding the naming? We need to >> > clarify the semantics in C, it seems. >> >> No other names have come in yet, in total the names proposed so far (I >> think) are: >> - maynotprogress >> - maybenoprogress >> - might_not_progress >> - nfpg >> - no_fpg >> and the loop metadata has been pretty firmly established as >> llvm.loop.mustprogress. IMHO, I've warmed up to no_fpg (or even nfpg >> in a pinch if we want to save 33% of space and lose some readability) >> since we've made substantial progress in clarifying the exact >> definitions of progress in this context and I think it's a good idea >> to bake it into the attribute name. > > > I'd actually like to suggest that we invert the default for functions. Rather than adding a "maynotprogress" function attribute, instead add a "mustprogress" function attribute, which Clang will emit on every function compiled in C++ mode. For two reasons: > 1. Having both the attribute and the loop metadata be the same way around makes it simpler to think about (rather than one being positive, and the other being negated). > 2. Given that the global progress-requirement seems to be pretty much C++-specific, having this behavior be off by default, and opted into by C++ frontends makes sense. > 3. Bonus: it makes choosing an attribute name easier: mustprogress, done. > >> I've also modified the clang patch [0] to only apply either of the >> >> attributes for C functions when compiled with C11 or later so we can >> tightly adhere to both the C and C++ standards, and the other changes >> that need to be made will be forthcoming. Thanks again to James, that >> particular example was pretty cool, and I agree that it may be best to >> follow that interpretation. >> >> [0] https://reviews.llvm.org/D86841 > > > You mean that you now apply maynotprogress to all functions in C, right? But why only C11 and later? I think all versions of C should get the maynotprogress function attribute? (Or, with the change I suggest above: only C++ code should get the "mustprogress" function attribute.)I actually couldn't find that particular statement in the C99 spec or before, so all functions in C when compiled with C11 or later. So with the changes you suggested, it'd be pre-C11 and C++ with the "mustprogress".
James Y Knight via llvm-dev
2020-Sep-11 19:54 UTC
[llvm-dev] [RFC] Introducing the maynotprogress IR attribute
On Fri, Sep 11, 2020 at 2:33 PM Atmn Patel <atmndp at gmail.com> wrote:> Hi James, > > On Fri, Sep 11, 2020 at 2:05 PM James Y Knight <jyknight at google.com> > wrote: > > > > On Fri, Sep 11, 2020 at 12:42 PM Atmn Patel <atmndp at gmail.com> wrote: > >> > >> Hi Hal, > >> > >> On Thu, Sep 10, 2020 at 8:54 PM Hal Finkel <hfinkel at anl.gov> wrote: > >> > > >> > Hi, Atmn, > >> > > >> > Has anyone else expressed an opinion regarding the naming? We need to > >> > clarify the semantics in C, it seems. > >> > >> No other names have come in yet, in total the names proposed so far (I > >> think) are: > >> - maynotprogress > >> - maybenoprogress > >> - might_not_progress > >> - nfpg > >> - no_fpg > >> and the loop metadata has been pretty firmly established as > >> llvm.loop.mustprogress. IMHO, I've warmed up to no_fpg (or even nfpg > >> in a pinch if we want to save 33% of space and lose some readability) > >> since we've made substantial progress in clarifying the exact > >> definitions of progress in this context and I think it's a good idea > >> to bake it into the attribute name. > > > > > > I'd actually like to suggest that we invert the default for functions. > Rather than adding a "maynotprogress" function attribute, instead add a > "mustprogress" function attribute, which Clang will emit on every function > compiled in C++ mode. For two reasons: > > 1. Having both the attribute and the loop metadata be the same way > around makes it simpler to think about (rather than one being positive, and > the other being negated). > > 2. Given that the global progress-requirement seems to be pretty much > C++-specific, having this behavior be off by default, and opted into by C++ > frontends makes sense. > > 3. Bonus: it makes choosing an attribute name easier: mustprogress, done. > > > >> I've also modified the clang patch [0] to only apply either of the > >> > >> attributes for C functions when compiled with C11 or later so we can > >> tightly adhere to both the C and C++ standards, and the other changes > >> that need to be made will be forthcoming. Thanks again to James, that > >> particular example was pretty cool, and I agree that it may be best to > >> follow that interpretation. > >> > >> [0] https://reviews.llvm.org/D86841 > > > > > > You mean that you now apply maynotprogress to all functions in C, right? > But why only C11 and later? I think all versions of C should get the > maynotprogress function attribute? (Or, with the change I suggest above: > only C++ code should get the "mustprogress" function attribute.) > > I actually couldn't find that particular statement in the C99 spec or > before, so all functions in C when compiled with C11 or later. So with > the changes you suggested, it'd be pre-C11 and C++ with the > "mustprogress". >If C99 doesn't have any statement about progress being required that doesn't imply that progress is always required. Rather the opposite -- that progress was not ever required until C11. So, we shouldn't add any mustprogress annotations in C89/99. It also looks as though the C++ text was only added in C++11, so we also shouldn't add any mustprogress annotations in C++98/03 modes. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200911/95ba3cc5/attachment.html>
Atmn Patel via llvm-dev
2020-Oct-13 16:39 UTC
[llvm-dev] [RFC] Introducing the maynotprogress IR attribute
Hi All, Based on the discussion, we changed the implementation so that there is a `mustprogress` function attribute that is applied to all C++ functions that do not contain a loop with a non-zero constant conditional, and a loop metadata `llvm.loop.mustprogress` that is applied to each loop without a constant conditional when compiled with standards C11/C++11 or later. The relevant patches are: D85393, D86233, D87262, D88464, D86841, and D86844, some of which have already been accepted, and are ready to be upstreamed, and I will start doing so shortly pending no additional design comments. There will be follow-up patches for fixing more miscompilations that are currently done by the existing passes. Atmn On Fri, Sep 11, 2020 at 3:55 PM James Y Knight <jyknight at google.com> wrote:> > > On Fri, Sep 11, 2020 at 2:33 PM Atmn Patel <atmndp at gmail.com> wrote: > >> Hi James, >> >> On Fri, Sep 11, 2020 at 2:05 PM James Y Knight <jyknight at google.com> >> wrote: >> > >> > On Fri, Sep 11, 2020 at 12:42 PM Atmn Patel <atmndp at gmail.com> wrote: >> >> >> >> Hi Hal, >> >> >> >> On Thu, Sep 10, 2020 at 8:54 PM Hal Finkel <hfinkel at anl.gov> wrote: >> >> > >> >> > Hi, Atmn, >> >> > >> >> > Has anyone else expressed an opinion regarding the naming? We need to >> >> > clarify the semantics in C, it seems. >> >> >> >> No other names have come in yet, in total the names proposed so far (I >> >> think) are: >> >> - maynotprogress >> >> - maybenoprogress >> >> - might_not_progress >> >> - nfpg >> >> - no_fpg >> >> and the loop metadata has been pretty firmly established as >> >> llvm.loop.mustprogress. IMHO, I've warmed up to no_fpg (or even nfpg >> >> in a pinch if we want to save 33% of space and lose some readability) >> >> since we've made substantial progress in clarifying the exact >> >> definitions of progress in this context and I think it's a good idea >> >> to bake it into the attribute name. >> > >> > >> > I'd actually like to suggest that we invert the default for functions. >> Rather than adding a "maynotprogress" function attribute, instead add a >> "mustprogress" function attribute, which Clang will emit on every function >> compiled in C++ mode. For two reasons: >> > 1. Having both the attribute and the loop metadata be the same way >> around makes it simpler to think about (rather than one being positive, and >> the other being negated). >> > 2. Given that the global progress-requirement seems to be pretty much >> C++-specific, having this behavior be off by default, and opted into by C++ >> frontends makes sense. >> > 3. Bonus: it makes choosing an attribute name easier: mustprogress, >> done. >> > >> >> I've also modified the clang patch [0] to only apply either of the >> >> >> >> attributes for C functions when compiled with C11 or later so we can >> >> tightly adhere to both the C and C++ standards, and the other changes >> >> that need to be made will be forthcoming. Thanks again to James, that >> >> particular example was pretty cool, and I agree that it may be best to >> >> follow that interpretation. >> >> >> >> [0] https://reviews.llvm.org/D86841 >> > >> > >> > You mean that you now apply maynotprogress to all functions in C, >> right? But why only C11 and later? I think all versions of C should get the >> maynotprogress function attribute? (Or, with the change I suggest above: >> only C++ code should get the "mustprogress" function attribute.) >> >> I actually couldn't find that particular statement in the C99 spec or >> before, so all functions in C when compiled with C11 or later. So with >> the changes you suggested, it'd be pre-C11 and C++ with the >> "mustprogress". >> > > If C99 doesn't have any statement about progress being required that > doesn't imply that progress is always required. Rather the opposite -- that > progress was not ever required until C11. So, we shouldn't add any > mustprogress annotations in C89/99. > > It also looks as though the C++ text was only added in C++11, so we also > shouldn't add any mustprogress annotations in C++98/03 modes. > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201013/481bcdc4/attachment.html>