llvm dev - Sep 2020 - [RFC] Introducing the maynotprogress IR attribute

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'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
>   -Hal
>
> On 9/10/20 1:21 PM, Atmn Patel wrote:
> > Hi Nicolai and Hal,
> >
> > I was wondering if your present concerns regarding the directions of
> > the proposed attributes and semantics of the current direction had
> > been addressed, so I thought I'd send over a friendly ping. Have
they?
> >
> > Atmn
> >
> > On Wed, Sep 9, 2020 at 1:08 AM Johannes Doerfert
> > <johannesdoerfert at gmail.com> wrote:
> >>
> >> On 9/8/20 9:08 PM, Hal Finkel wrote:
> >>   > There is another thing that I would like for us to
document: do
> >>   > infinite loops have any relationship to thread
synchronization? As I
> >>   > recall, this issue came up in the past when we discussed
the deletion,
> >>   > or not, of infinite loops. If a thread writes a value to
memory, and
> >>   > then enters an infinite loop, is that value eventually
visible to
> >>   > other threads? My understanding is that some code depends
on this
> >>   > property. If this is something that people would like to
see
> >>   > guaranteed,
> >>
> >> I doubt we guarantee that now, though we might not actively
exploit it.
> >> I also think we should not treat termination as synchronization,
at
> >> least not in the case where progress is required. So, in C (and
arguably
> >> we might want to do this also in C++), we would say that:
> >> ```
> >> global_signal = 1;
> >> while (1);
> >> ```
> >> is well defined and we will not remove the write.
> >>
> >> FWIW, I would agree that we should write this down though.
> >>
> >>
> >>   > I suspect that we may need specific handling (if nothing
> >>   > else, so we don't sink stores past possibly-infinite
loops).
> >>
> >> If (possibly-)infinite loops are well defined, e.g., in a function
with
> >> the `mayprogress` attribute, we certainly cannot move any effect
past
> >> them. If they are not, they are UB if they loop infinitely and
otherwise
> >> finite and therefore a no-op. So we can either delete them, by
assuming
> >> they are finite, or, if we know they are not, actually eliminate
> >> anything that is known to transfer execution to the loop [0].
> >>
> >> [0] https://reviews.llvm.org/D87149
> >>
> >> ~ Johannes
> >>
> >>   >  -Hal
> >>
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
>

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.)
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200911/3255b481/attachment.html>

On 9/11/20 1:04 PM, James Y Knight wrote:
> On Fri, Sep 11, 2020 at 12:42 PM Atmn Patel <atmndp at gmail.com 
> <mailto:atmndp at gmail.com>> wrote:
>
>     Hi Hal,
>
>     On Thu, Sep 10, 2020 at 8:54 PM Hal Finkel <hfinkel at anl.gov
>     <mailto: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 support this suggestion.

  -Hal

>
>     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
<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.)
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200911/acce3eb9/attachment.html>

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

On 9/11/20 1:04 PM, James Y Knight via llvm-dev 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.

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

 > 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.)
 >
 >
 > _______________________________________________
 > LLVM Developers mailing list
 > llvm-dev at lists.llvm.org
 > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev