<div dir="ltr">Hi All,<br><br>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.<br><br>Atmn</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 11, 2020 at 3:55 PM James Y Knight <<a href="mailto:jyknight@google.com">jyknight@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 11, 2020 at 2:33 PM Atmn Patel <<a href="mailto:atmndp@gmail.com" target="_blank">atmndp@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi James,<br>
<br>
On Fri, Sep 11, 2020 at 2:05 PM James Y Knight <<a href="mailto:jyknight@google.com" target="_blank">jyknight@google.com</a>> wrote:<br>
><br>
> On Fri, Sep 11, 2020 at 12:42 PM Atmn Patel <<a href="mailto:atmndp@gmail.com" target="_blank">atmndp@gmail.com</a>> wrote:<br>
>><br>
>> Hi Hal,<br>
>><br>
>> On Thu, Sep 10, 2020 at 8:54 PM Hal Finkel <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>> wrote:<br>
>> ><br>
>> > Hi, Atmn,<br>
>> ><br>
>> > Has anyone else expressed an opinion regarding the naming? We need to<br>
>> > clarify the semantics in C, it seems.<br>
>><br>
>> No other names have come in yet, in total the names proposed so far (I<br>
>> think) are:<br>
>> - maynotprogress<br>
>> - maybenoprogress<br>
>> - might_not_progress<br>
>> - nfpg<br>
>> - no_fpg<br>
>> and the loop metadata has been pretty firmly established as<br>
>> llvm.loop.mustprogress. IMHO, I've warmed up to no_fpg (or even nfpg<br>
>> in a pinch if we want to save 33% of space and lose some readability)<br>
>> since we've made substantial progress in clarifying the exact<br>
>> definitions of progress in this context and I think it's a good idea<br>
>> to bake it into the attribute name.<br>
><br>
><br>
> 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:<br>
> 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).<br>
> 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.<br>
> 3. Bonus: it makes choosing an attribute name easier: mustprogress, done.<br>
><br>
>> I've also modified the clang patch [0] to only apply either of the<br>
>><br>
>> attributes for C functions when compiled with C11 or later so we can<br>
>> tightly adhere to both the C and C++ standards, and the other changes<br>
>> that need to be made will be forthcoming. Thanks again to James, that<br>
>> particular example was pretty cool, and I agree that it may be best to<br>
>> follow that interpretation.<br>
>><br>
>> [0] <a href="https://reviews.llvm.org/D86841" rel="noreferrer" target="_blank">https://reviews.llvm.org/D86841</a><br>
><br>
><br>
> 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.)<br>
<br>
I actually couldn't find that particular statement in the C99 spec or<br>
before, so all functions in C when compiled with C11 or later. So with<br>
the changes you suggested, it'd be pre-C11 and C++ with the<br>
"mustprogress".<br></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div></div></div>
</blockquote></div>