Johannes Doerfert via llvm-dev
2020-Sep-07 21:15 UTC
[llvm-dev] [RFC] Introducing the maynotprogress IR attribute
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 >> > >> > >> > >> > >
Nicolai Hähnle via llvm-dev
2020-Sep-07 21:48 UTC
[llvm-dev] [RFC] Introducing the maynotprogress IR attribute
Hi Johannes, On Mon, Sep 7, 2020 at 11:17 PM Johannes Doerfert <johannesdoerfert at gmail.com> wrote:> >> > 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.The point is purely one about understandability of the proposed LangRef change. The LangRef change talks about "progress" as defined in [intro.progress] of the C++ spec, but the vast majority of that section, and _all_ parts of that section that actually use the word "progress", talk about the kind of forward progress guarantees that have nothing at all to do with what you're trying to get at in the proposal. My understanding is that for your proposal here, you're only really interested in what's written in the very first clause of that C++ spec section, but that's totally non-obvious from what the proposed patch to the LangRef says. The point is that the proposed language is misleading to somebody who approaches it without preconceptions. Cheers, Nicolai> > ~ 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 > >> > > >> > > >> > > >> > > > > >-- Lerne, wie die Welt wirklich ist, aber vergiss niemals, wie sie sein sollte.
Johannes Doerfert via llvm-dev
2020-Sep-07 22:38 UTC
[llvm-dev] [RFC] Introducing the maynotprogress IR attribute
On 9/7/20 4:48 PM, Nicolai Hähnle wrote:> Hi Johannes, > > > On Mon, Sep 7, 2020 at 11:17 PM Johannes Doerfert > <johannesdoerfert at gmail.com> wrote: >> >> > 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. > The point is purely one about understandability of the proposed > LangRef change. The LangRef change talks about "progress" as defined > in [intro.progress] of the C++ spec, but the vast majority of that > section, and _all_ parts of that section that actually use the word > "progress", talk about the kind of forward progress guarantees that > have nothing at all to do with what you're trying to get at in the > proposal. My understanding is that for your proposal here, you're only > really interested in what's written in the very first clause of that > C++ spec section, but that's totally non-obvious from what the > proposed patch to the LangRef says. The point is that the proposed > language is misleading to somebody who approaches it without > preconceptions.I guess we can say something like: [6, see the first section under the title "forward progress"] if you think that makes it easier to read. Would that address your concern or did I still not understand? ~ Johannes> Cheers, > Nicolai > > > > >> ~ 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 >> >> > >> >> > >> >> > >> >> >> > >> > >> >
James Y Knight via llvm-dev
2020-Sep-10 21:49 UTC
[llvm-dev] [RFC] Introducing the maynotprogress IR attribute
On Mon, Sep 7, 2020 at 5:17 PM Johannes Doerfert via llvm-dev < llvm-dev at lists.llvm.org> wrote:> 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.So, just to reiterate, the proposal is that Clang will look at each function, and: - if a function contains any loops which may not progress, will emit "maynotprogress" attribute on the function definition, and "mustprogress" metadata on each loop other than those that may be infinite. - otherwise, it will not emit the function attribute "maynotprogress", and will not emit "mustprogress" metadata on the loops. And then, whenever LLVM inlines code into another function: - If the outer function has maynotprogress, and the inner does not: add mustprogress metadata to all the loops (?) from the inner function. - If the outer function does not have maynotprogress, and the inner does: add maynotprogress to the outer function, and add mustprogress to all loops in the outer. (But, how do you identify the "loop" to put the metadata on?) An alternative scheme suggested was to do without the function attribute, and for clang to emit "mustprogress" metadata on every loop which has this property. Some potential reasons to want not to do the simpler scheme have been expressed: - Would deoptimize code emitted by previous frontends which don't know about mustprogress. - Concern that the loop metadata may be lost during other transforms, thus losing the optimization. I'd like to raise another issue, that I haven't seen anyone bring up yet -- the definition of a "loop" in the context of these "infinite loop" optimizations does not seem to have been made clear, and, I think, is problematic. In the current state of LLVM, I believe a candidate "loop" to delete can be derived from *any* control flow, it does not need to be derived from an explicit source-language loop construct. Under this proposal, that would still be the case. (unless the function was annotated with maynotprogress.) For C code, that is incorrect. C only has a must-progress requirement explicitly for iteration statements ("for", "while", and "do"). (And of those, only ones which do not have a constant controlling expression). Quoting C11, 6.8.5 Iteration statements, p6, "An iteration statement whose controlling expression is not a constant expression, that performs no input/output operations, does not access volatile objects, and performs no synchronization or atomic operations in its body, controlling expression, or (in the case of for statement) its expression-3, may be assumed by the implementation to terminate." So, compiling C code, ISTM that every C function needs to be "maynotprogress", and *only* for "for", "while", or "do" loops without a constant loop-controlling-expression, can we add the "mustprogress" metadata. On the other hand, in C++, it would appear that by [intro.progress] there is a guarantee of progress for *all* control flow in a function, even if you've built it out of explicit goto statements. That is, I believe in C++ this function can be optimized away, but not in C. void loop_weirdly(int should_loop) { label: if (should_loop) goto label; } While this may be optimized away in both C and C++: void loop(int should_loop) { while (should_loop) ; } Another question -- can we actually be sure that other control flow won't get merged into the loop -- is it possible for the "mustprogress" metadata to get applied to a wider scope than it should've been, after other control flow optimizations occur? -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200910/025866fe/attachment.html>
Johannes Doerfert via llvm-dev
2020-Sep-10 22:26 UTC
[llvm-dev] [RFC] Introducing the maynotprogress IR attribute
Please scroll to the second to last paragraph for an action item! On 9/10/20 4:49 PM, James Y Knight wrote: > On Mon, Sep 7, 2020 at 5:17 PM Johannes Doerfert via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> 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. > > > So, just to reiterate, the proposal is that Clang will look at each > function, and: > - if a function contains any loops which may not progress, will emit > "maynotprogress" attribute on the function definition, Yes. See also below. > and "mustprogress" metadata on each loop other than those that may be > infinite. The metadata is added to loops that have to make progress but are in a function in which loops do not have to make progress. It is not "needed" but an optimization. Example: void foo(int N) { for(int i = 0; i < N; ++i) // <- loop.metadata bar(i); int M = N + 1; while (N < M); // <- loop.metadata, would be removed if (N) while (1); while (N < M); // <- loop.metadata, would be removed } > - otherwise, it will not emit the function attribute "maynotprogress", and > will not emit "mustprogress" metadata on the loops. Yes. If there is no loop in the code with well-defined semantics if it is infinite, Clang won't do anything different. Loops with well-defined semantics if they are infinite are C loops with a constant conditional (=true), we could include all loops (C++,...) with a constant conditional while we are at it IMHO. > And then, whenever LLVM inlines code into another function: > - If the outer function has maynotprogress, and the inner does not: add > mustprogress metadata to all the loops (?) from the inner function. Right. We don't have the patch for this yet I think. > - If the outer function does not have maynotprogress, and the inner does: > add maynotprogress to the outer function, and add mustprogress to all loops > in the outer. Right. We have a patch for this. > (But, how do you identify the "loop" to put the metadata on?) For now, LoopInfo. There is no need to get them all, we can miss some. To be fair, we don't optimize "loops" that are not recognized by LoopInfo. > An alternative scheme suggested was to do without the function attribute, > and for clang to emit "mustprogress" metadata on every loop which has this > property. Right: We can make the universal default `maynotprogress`, so the loops are well-defined if they are infinite and do nothing. (That means calls that do not have side effects are then something that cannot be removed if they might loop indefinitely.) Then we must "opt-in" to all optimizations, e.g., loop-deletion and more importantly call deletion. There were ideas to use an attribute to "opt-in" and some to use metadata, or both. > Some potential reasons to want not to do the simpler scheme have been > expressed: > - Would deoptimize code emitted by previous frontends which don't know > about mustprogress. > - Concern that the loop metadata may be lost during other transforms, thus > losing the optimization. > > > I'd like to raise another issue, that I haven't seen anyone bring up yet -- > the definition of a "loop" in the context of these "infinite loop" > optimizations does not seem to have been made clear, and, I think, is > problematic. In the current state of LLVM, I believe a candidate "loop" to > delete can be derived from *any* control flow, it does not need to be > derived from an explicit source-language loop construct. Under this > proposal, that would still be the case. (unless the function was annotated > with maynotprogress.) > > For C code, that is incorrect. C only has a must-progress requirement > explicitly for iteration statements ("for", "while", and "do"). (And of > those, only ones which do not have a constant controlling expression). > Quoting C11, 6.8.5 Iteration statements, p6, "An iteration statement whose > controlling expression is not a constant expression, that performs no > input/output operations, does not access volatile objects, and performs no > synchronization or atomic operations in its body, controlling expression, > or (in the case of for statement) its expression-3, may be assumed by the > implementation to terminate." > > So, compiling C code, ISTM that every C function needs to be > "maynotprogress", and *only* for "for", "while", or "do" loops without a > constant loop-controlling-expression, can we add the "mustprogress" > metadata. > > On the other hand, in C++, it would appear that by [intro.progress] there > is a guarantee of progress for *all* control flow in a function, even if > you've built it out of explicit goto statements. > > That is, I believe in C++ this function can be optimized away, but not in C. > void loop_weirdly(int should_loop) { > label: > if (should_loop) goto label; > } > > While this may be optimized away in both C and C++: > void loop(int should_loop) { > while (should_loop) ; > } This example is great; the compiler council [0] is divided: Clang + icc*: keep the loop in loop_weirdly but remove calls to it, both in C and C++. gcc: follows your interpretation, in C the termination is well-defined behavior and both the loop in loop_weirdly as well as a call of it are kept. In C++ both are removed. MSVC: plays it conservative and doesn't remove anything in either mode. *icc13 removes the loop in loop_weirdly. I was going to provide options what this means for this RFC and existing optimizations we do right now, but I think it would be better if people would first chime in and we agree on what behavior is "correct". Thanks Johannes [0] https://godbolt.org/z/KosGnE > Another question -- can we actually be sure that other control flow won't > get merged into the loop -- is it possible for the "mustprogress" metadata > to get applied to a wider scope than it should've been, after other control > flow optimizations occur? I think we can be sure this is OK if all transformations behave as they already have to. While you can imagine a transformation changing the loop in different ways, let's say fusion, metadata cannot be kept if it is potentially invalidated. This is already true for all the other ones we have, e.g., to override the vectorizer legality checks.