[RFC] Introducing the maynotprogress IR attribute

Hi Hal,

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.)

    Hi Hal,

    >
    > 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 support this suggestion.

-Hal

Hi James,

>> Hi Hal,
>>
>>>
>>> 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.

Yeah, since we basically miscompile all but C++ code right now it makes
sense to switch the IR default and make C++ "special". To actually stop
miscompiling these codes we need to change more places as they have the
`mustprogress` backed in, though that was necessary either way. (We
could say that a call w/o the `mustprogress` and w/o `willreturn` is a
side-effect of sorts.)

~ Johannes

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.

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