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

On 9/7/20 10:56 AM, Nicolai Hähnle wrote:
 > Hi Johannes and Atmn,
 >
 > On Sat, Sep 5, 2020 at 7:07 AM Johannes Doerfert via llvm-dev
 > <llvm-dev at lists.llvm.org> wrote:
 >>  > In any case, please explain the intended behavior of the 
attribute and
 >>  > the metadata upon inlining.
 >>
 >> The attribute will be attached to the caller upon inlining as this is
 >> the only way to keep semantics correct. Metadata can be added to the
 >> loops of the caller if the caller did not have the attribute, but that
 >> is an optimization and not required. The patch for the first part will
 >> be part of this change set.
 >
 > I don't understand why this would be correct, unless you meant this
 > statement to only refer to loops in the caller that contain the
 > inlined call.

I'm not sure I interpret "loops in the caller that contain the inlined
call" correctly but let me give it a try.

So this is the first inliner patch: https://reviews.llvm.org/D87180
It is alone sufficient for correctness of this scheme. A follow up can
provide better optimization opportunities by using metadata to annotate
loops in the caller, assuming the caller did not have the
`maynotprogress` function attribute already. All loops originally in the
caller would be given the loop metadata that indicates the new
`maynotprogress` inherited from the callee does not apply to them. If
the metadata is lost for some reason, we do not loose correctness.


 > A deeper issue I have with the proposal is that I'm concerned about
 > adding more attributes which add constraints on program transforms.

Let me start with something that seems to get lost:
We miscompile C/C++ programs right now and we do not apply
transformations to hide the issue one level.

That is,
```C
while(1);
```
is *not* deleted by LoopDeletion right now, which is correct.

Though,
```C
int b = a + 1;
while(b > a);
```
is *not* deleted by LoopDeletion either, which is not necessary and a
side effect of our existing failure to distinguish the two.

Finally,
```
void foo() { while (1); }
void bar() { foo(); }
```
can be optimized by us to `void bar() {}` which is *incorrect*.

So, to summarize, we are in a situation in which we constraint program
transformations, perform wrong program transformations, and require
languages like Rust to add spurious side-effects to avoid this.


 > I'm afraid I don't have the full history of these discussions, but
is
 > there really a _good_ reason for doing so? It would feel more natural
 > to have no progress requirement by default and add a
"willprogress"
 > attribute to indicate that the so annotated function will eventually
 > make progress (whatever that means, which is the other issue I have
 > with this :))

We can certainly turn everything around. Though, that would basically
regress our current IR and the "normal" C/C++ case (which is forward
progress requirement). So if people feel we want to make the IR more
restrictive than it is arguably right now, with an opt-in way to get the
current semantics back, I won't oppose that. However, all conversations
up to this point seemed to indicate that we do not want this. Please see
[1] and [5].


 > Assuming the simple flip of polarity from the previous paragraph, what
 > is the difference between the existing "willreturn" and the
proposed
 > "willprogress"? Furthermore, if there is a difference, why is it
 > relevant to the compiler?

I mean there is plenty of cases where you have one but not the other:
```
while (1) {
   atomic_add(X, 1);
}
```
does make progress but will not return. More importantly, willreturn is
a property we derive, willprogress is a property of the input language
embedded in the IR. We can use the latter to derive the former, sure,
but only one of them is rooted in (high-level) language semantics.

We want `willreturn` to improve
isGuaranteedToTransferExecutionToSuccessor (or something like that)
which then has various uses.

Take
```
foo();
*ptr = 0;
```
If we know `foo` is `willreturn` and `nounwind` we know `ptr` is
`derefereceable` prior to the call (on most targets). This allows to
hoist/sink code, improves alias analysis, ... On the other end, with
https://reviews.llvm.org/rGb22910daab95 we can remove more dead code
based on `willreturn` (and `nounwind`).


 > Assuming the simple flip of polarity, is a new attribute needed at
 > all? It seems like the "mustprogress" loop metadata would be
 > sufficient.

I think that is correct. Though, then we are back the regressing all
existing IR. I'm not opposing this choice but it seems to me of little
gain.

To put the regression into perspective: We would basically need to stop
removing an unused `readonly` + `nounwind` call as it could loop
indefinitely and that would be well defined. In the proposed scheme you
would not remove them if they carry the `maynotprogress` attribute, and
you can drop the `maynotprogress` attribute once you proven it will make
progress, e.g., once you proven `willreturn`.


 > As a separate comment, I don't find the reference to the C++ spec in
 > https://reviews.llvm.org/D86233 to be informative enough. Whenever
 > that section of the C++ spec talks about "progress" it is
referring to
 > how some abstract scheduler schedules execution of multiple threads.
 > That is, it is talking about the dynamic behavior of the
 > implementation. On the other hand, the proposed attribute presumably
 > makes a statement about the program itself _assuming that_ its
 > execution gets scheduled. So there is no applicable definition of
 > "progress" in the cited section of the C++ spec.

I don't understand. What is the alternative to "assuming that its
execution gets scheduled"?

~ Johannes


 > Cheers,
 > Nicolai
 >
 >
 >>  >> [1] https://bugs.llvm.org/show_bug.cgi?id=965
 >>  >> [2]
https://lists.llvm.org/pipermail/llvm-dev/2015-July/088103.html
 >>  >> [3] 
https://lists.llvm.org/pipermail/llvm-dev/2017-October/118558.html
 >>  >> [4] https://reviews.llvm.org/D38336
 >>  >> [5] https://reviews.llvm.org/D65718
 >>  >> [6] https://eel.is/c++draft/intro.progress
 >
 >
 >

Hi Johannes,

On Mon, Sep 7, 2020 at 6:51 PM Johannes Doerfert
<johannesdoerfert at gmail.com> wrote:
> On 9/7/20 10:56 AM, Nicolai Hähnle wrote:
>  > Hi Johannes and Atmn,
>  >
>  > On Sat, Sep 5, 2020 at 7:07 AM Johannes Doerfert via llvm-dev
>  > <llvm-dev at lists.llvm.org> wrote:
>  >>  > In any case, please explain the intended behavior of the
> attribute and
>  >>  > the metadata upon inlining.
>  >>
>  >> The attribute will be attached to the caller upon inlining as
this is
>  >> the only way to keep semantics correct. Metadata can be added to
the
>  >> loops of the caller if the caller did not have the attribute, but
that
>  >> is an optimization and not required. The patch for the first part
will
>  >> be part of this change set.
>  >
>  > I don't understand why this would be correct, unless you meant
this
>  > statement to only refer to loops in the caller that contain the
>  > inlined call.
>
> I'm not sure I interpret "loops in the caller that contain the
inlined
> call" correctly but let me give it a try.
>
> So this is the first inliner patch: https://reviews.llvm.org/D87180
> It is alone sufficient for correctness of this scheme. A follow up can
> provide better optimization opportunities by using metadata to annotate
> loops in the caller, assuming the caller did not have the
> `maynotprogress` function attribute already. All loops originally in the
> caller would be given the loop metadata that indicates the new
> `maynotprogress` inherited from the callee does not apply to them. If
> the metadata is lost for some reason, we do not loose correctness.
Okay, thank you for that first explanation. It does feel like there's
some redundancy there, though. Why weren't all those loops annotated
with metadata in the first place?

>  > I'm afraid I don't have the full history of these
discussions, but is
>  > there really a _good_ reason for doing so? It would feel more natural
>  > to have no progress requirement by default and add a
"willprogress"
>  > attribute to indicate that the so annotated function will eventually
>  > make progress (whatever that means, which is the other issue I have
>  > with this :))
>
> We can certainly turn everything around. Though, that would basically
> regress our current IR and the "normal" C/C++ case (which is
forward
> progress requirement). So if people feel we want to make the IR more
> restrictive than it is arguably right now, with an opt-in way to get the
> current semantics back, I won't oppose that. However, all conversations
> up to this point seemed to indicate that we do not want this. Please see
> [1] and [5].
Those discussions are fairly long and I didn't see a concrete argument
against changing the default in them. I may have just missed it, but I
also wonder if it's one of those cases where people in the LLVM
community are too timid, leading us to muddle through with suboptimal
designs.

If changing the default allows us to get away without a new attribute
and redundant representations of semantics, then that's an overall
simpler design in the long run, which should weigh _very_ heavily IMO.

That said, [5] mentions the problem of recursion, which suggests that
perhaps even with a changed default, a solution purely based on
metadata _wouldn't_ be enough? Obviously you could put metadata on
function calls, but that's not usually how we do that kind of thing.
So even with the changed default, it seems like a "willprogress"
attribute might be required? It's admittedly not clear to me.

There is still the argument that it's annoying to have attributes that
_forbid_ program transforms, when virtually all attributes _allow_
program transforms instead. The only other example I can think of
right now is "convergent".

>  > Assuming the simple flip of polarity from the previous paragraph,
what
>  > is the difference between the existing "willreturn" and the
proposed
>  > "willprogress"? Furthermore, if there is a difference, why
is it
>  > relevant to the compiler?
>
> I mean there is plenty of cases where you have one but not the other:
> ```
> while (1) {
>    atomic_add(X, 1);
> }
> ```
> does make progress but will not return. More importantly, willreturn is
> a property we derive, willprogress is a property of the input language
> embedded in the IR. We can use the latter to derive the former, sure,
> but only one of them is rooted in (high-level) language semantics.
"willreturn" could be stated in the source language.

> We want `willreturn` to improve
> isGuaranteedToTransferExecutionToSuccessor (or something like that)
> which then has various uses.
[snip]

Yes, the question was really more about what a "willprogress" buys us.
(Other than as a function-wide default for loop metadata which could
get lost, I suppose -- but we've been going away from function-wide
defaults for a while, e.g. fast math flags, not to mention that
redundant encodings of semantics are generally a bad thing.)

Perhaps something about recursion, as mentioned above? That's not clear to
me.

>  > As a separate comment, I don't find the reference to the C++ spec
in
>  > https://reviews.llvm.org/D86233 to be informative enough. Whenever
>  > that section of the C++ spec talks about "progress" it is
referring to
>  > how some abstract scheduler schedules execution of multiple threads.
>  > That is, it is talking about the dynamic behavior of the
>  > implementation. On the other hand, the proposed attribute presumably
>  > makes a statement about the program itself _assuming that_ its
>  > execution gets scheduled. So there is no applicable definition of
>  > "progress" in the cited section of the C++ spec.
>
> I don't understand. What is the alternative to "assuming that its
> execution gets scheduled"?
The alternative is dealing with the possibility that execution of a
thread is _not_ scheduled by the abstract machine :)

This is a serious topic on GPUs, where you may have no or very weak
forward progress guarantees, e.g. because threads mapped to the same
vector/wave might not make progress while the vector is executing some
other divergent part of the CFG, or because more threads are requested
than physically fit on the machine.

Cheers,
Nicolai


>
> ~ Johannes
>
>
>  > Cheers,
>  > Nicolai
>  >
>  >
>  >>  >> [1] https://bugs.llvm.org/show_bug.cgi?id=965
>  >>  >> [2]
https://lists.llvm.org/pipermail/llvm-dev/2015-July/088103.html
>  >>  >> [3]
> https://lists.llvm.org/pipermail/llvm-dev/2017-October/118558.html
>  >>  >> [4] https://reviews.llvm.org/D38336
>  >>  >> [5] https://reviews.llvm.org/D65718
>  >>  >> [6] https://eel.is/c++draft/intro.progress
>  >
>  >
>  >
>

-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.

On 9/7/20 2:52 PM, Nicolai Hähnle wrote:
 > Hi Johannes,
 >
 > On Mon, Sep 7, 2020 at 6:51 PM Johannes Doerfert
 > <johannesdoerfert at gmail.com> wrote:
 >> On 9/7/20 10:56 AM, Nicolai Hähnle wrote:
 >>  > Hi Johannes and Atmn,
 >>  >
 >>  > On Sat, Sep 5, 2020 at 7:07 AM Johannes Doerfert via llvm-dev
 >>  > <llvm-dev at lists.llvm.org> wrote:
 >>  >>  > In any case, please explain the intended behavior of
the
 >> attribute and
 >>  >>  > the metadata upon inlining.
 >>  >>
 >>  >> The attribute will be attached to the caller upon inlining
as
this is
 >>  >> the only way to keep semantics correct. Metadata can be
added to the
 >>  >> loops of the caller if the caller did not have the
attribute,
but that
 >>  >> is an optimization and not required. The patch for the first
part will
 >>  >> be part of this change set.
 >>  >
 >>  > I don't understand why this would be correct, unless you
meant this
 >>  > statement to only refer to loops in the caller that contain the
 >>  > inlined call.
 >>
 >> I'm not sure I interpret "loops in the caller that contain
the inlined
 >> call" correctly but let me give it a try.
 >>
 >> So this is the first inliner patch: https://reviews.llvm.org/D87180
 >> It is alone sufficient for correctness of this scheme. A follow up can
 >> provide better optimization opportunities by using metadata to
annotate
 >> loops in the caller, assuming the caller did not have the
 >> `maynotprogress` function attribute already. All loops originally in
the
 >> caller would be given the loop metadata that indicates the new
 >> `maynotprogress` inherited from the callee does not apply to them. If
 >> the metadata is lost for some reason, we do not loose correctness.
 >
 > Okay, thank you for that first explanation. It does feel like there's
 > some redundancy there, though. Why weren't all those loops annotated
 > with metadata in the first place?

Metadata to encode what exactly?

The idea is that loops which do not match the `maynotprogress` defined
via the attribute can be annotated. Thus, if the function is not
`maynotprogress` there is no need for the annotation. If the function
is, loops that do not match `maynotprogress` would at some point be
annotated, either by the frontend or inliner (= the two places that add
`maynotprogress`).

You cannot use metadata in the other direction, thus to opt-in to
`maynotprogress`. You can make `maynotprogress` the default and use
metadata to opt-out, sure. In addition to the regressions we might see,
we basically end up with metadata on almost every C/C++ loop, unsure if
that is a good idea.


 >>  > I'm afraid I don't have the full history of these
discussions, but is
 >>  > there really a _good_ reason for doing so? It would feel more
natural
 >>  > to have no progress requirement by default and add a
"willprogress"
 >>  > attribute to indicate that the so annotated function will
eventually
 >>  > make progress (whatever that means, which is the other issue I
have
 >>  > with this :))
 >>
 >> We can certainly turn everything around. Though, that would basically
 >> regress our current IR and the "normal" C/C++ case (which is
forward
 >> progress requirement). So if people feel we want to make the IR more
 >> restrictive than it is arguably right now, with an opt-in way to get
the
 >> current semantics back, I won't oppose that. However, all
conversations
 >> up to this point seemed to indicate that we do not want this. Please
see
 >> [1] and [5].
 >
 > Those discussions are fairly long and I didn't see a concrete argument
 > against changing the default in them. I may have just missed it, but I
 > also wonder if it's one of those cases where people in the LLVM
 > community are too timid, leading us to muddle through with suboptimal
 > designs.

I'm unsure if annotating every "normal" C/C++ loop and hoping loop
metadata survives is a superior design. At the end of the day the
question is where you want to put the cost (compile time and
optimization potential to some degree). Either make the presence of
well-defined non-progress loops more costly or the presence of undefined
non-progress loops. In our scheme the cost is minimal* if all of your
loops (in a function) fall in one or the other category. I would this to
be the "normal" case for all languages C/C++/Rust/... The design is
not
any worse than what you propose (assuming I understood) in case you mix
these loop kinds in a function. We basically fall-back to your scheme on
a function level if required. Admittedly this is more complex but it has
benefits and doesn't appear to be any less clean than the alternative.
We hide the query logic behind a helper function anyway.


* minimal cost: no metadata that needs to be maintained (and which could
                 be dropped) but full optimization potential

 > If changing the default allows us to get away without a new attribute
 > and redundant representations of semantics, then that's an overall
 > simpler design in the long run, which should weigh _very_ heavily IMO.
 >
 > That said, [5] mentions the problem of recursion, which suggests that
 > perhaps even with a changed default, a solution purely based on
 > metadata _wouldn't_ be enough? Obviously you could put metadata on
 > function calls, but that's not usually how we do that kind of thing.
 > So even with the changed default, it seems like a "willprogress"
 > attribute might be required? It's admittedly not clear to me.

Required for what?


 > There is still the argument that it's annoying to have attributes that
 > _forbid_ program transforms, when virtually all attributes _allow_
 > program transforms instead. The only other example I can think of
 > right now is "convergent".

An incomplete list of function attributes that disable transformations
(not actually checked but I expect them too):
   cold, convergent, noduplicate, noimplicitfloat, nomerge, noinline,
   nobuiltin, null_pointer_is_valid, optsize, optnone, returns_twice,
   "thunk"
Similarly there are parameter attributes that prevent transformations.


 >>  > Assuming the simple flip of polarity from the previous
paragraph,
what
 >>  > is the difference between the existing "willreturn"
and the proposed
 >>  > "willprogress"? Furthermore, if there is a difference,
why is it
 >>  > relevant to the compiler?
 >>
 >> I mean there is plenty of cases where you have one but not the other:
 >> ```
 >> while (1) {
 >>    atomic_add(X, 1);
 >> }
 >> ```
 >> does make progress but will not return. More importantly, willreturn
is
 >> a property we derive, willprogress is a property of the input language
 >> embedded in the IR. We can use the latter to derive the former, sure,
 >> but only one of them is rooted in (high-level) language semantics.
 >
 > "willreturn" could be stated in the source language.

I'm unsure what language has that guarantee. I don't think this is on
topic but feel free to provide me an example where a frontend, w/o
deduction, can use `willreturn` based on the language semantics.


 >> We want `willreturn` to improve
 >> isGuaranteedToTransferExecutionToSuccessor (or something like that)
 >> which then has various uses.
 > [snip]
 >
 > Yes, the question was really more about what a "willprogress"
buys us.
 > (Other than as a function-wide default for loop metadata which could
 > get lost, I suppose -- but we've been going away from function-wide
 > defaults for a while, e.g. fast math flags, not to mention that
 > redundant encodings of semantics are generally a bad thing.)

I feel like I mentioned this already a few times. It allows to a correct
representation of C/C++ (and other languages) without giving up on
optimizations we already do. Please clearly state if you would prefer us
to regress instead. I mentioned this already, reversing polarity means
we cannot remove calls that do not have the new attribute and, in
addition, all loops that do have the progress guarantee need annotations
(which need to be preserved).


 > Perhaps something about recursion, as mentioned above? That's not 
clear to me.
 >
 >
 >>  > As a separate comment, I don't find the reference to the C++
spec in
 >>  > https://reviews.llvm.org/D86233 to be informative enough.
Whenever
 >>  > that section of the C++ spec talks about "progress" it
is
referring to
 >>  > how some abstract scheduler schedules execution of multiple
threads.
 >>  > That is, it is talking about the dynamic behavior of the
 >>  > implementation. On the other hand, the proposed attribute
presumably
 >>  > makes a statement about the program itself _assuming that_ its
 >>  > execution gets scheduled. So there is no applicable definition
of
 >>  > "progress" in the cited section of the C++ spec.
 >>
 >> I don't understand. What is the alternative to "assuming that
its
 >> execution gets scheduled"?
 >
 > The alternative is dealing with the possibility that execution of a
 > thread is _not_ scheduled by the abstract machine :)

I guess if you do not schedule something there is not much to say about
it. We usually talk about about all executions and if you don't have
any, everything is true anyway. Unsure how you see that tie in here
though.

~ Johannes

 > This is a serious topic on GPUs, where you may have no or very weak
 > forward progress guarantees, e.g. because threads mapped to the same
 > vector/wave might not make progress while the vector is executing some
 > other divergent part of the CFG, or because more threads are requested
 > than physically fit on the machine.
 >
 > Cheers,
 > Nicolai
 >
 >
 >
 >>
 >> ~ Johannes
 >>
 >>
 >>  > Cheers,
 >>  > Nicolai
 >>  >
 >>  >
 >>  >>  >> [1] https://bugs.llvm.org/show_bug.cgi?id=965
 >>  >>  >> [2] 
https://lists.llvm.org/pipermail/llvm-dev/2015-July/088103.html
 >>  >>  >> [3]
 >> https://lists.llvm.org/pipermail/llvm-dev/2017-October/118558.html
 >>  >>  >> [4] https://reviews.llvm.org/D38336
 >>  >>  >> [5] https://reviews.llvm.org/D65718
 >>  >>  >> [6] https://eel.is/c++draft/intro.progress
 >>  >
 >>  >
 >>  >
 >>
 >
 >