[PATCH] D60074: [Attributor] Deduce "no-recurse" function attribute

[Nick Lewycky via Phabricator via llvm-commits]

nicholas added a comment.

In D60074#1533285 <https://reviews.llvm.org/D60074#1533285>, @jdoerfert wrote:

> Thanks for the extensive comment!
>
> In D60074#1532046 <https://reviews.llvm.org/D60074#1532046>, @nicholas wrote:
>
> > In D60074#1531819 <https://reviews.llvm.org/D60074#1531819>, @jdoerfert wrote:
> >
> > > In D60074#1531809 <https://reviews.llvm.org/D60074#1531809>, @efriedma wrote:
> > >
> > > > Is it actually correct to derive norecurse like this?
> > > >
> > > > Suppose I have two functions:
> > > >
> > > >   void f(int x) { if (x) { g(); } }
> > > >   void g() { f(0); }
> > > >
> > > >
> > > > As far as I can tell, it would be correct to mark g() norecurse.  And if g() is marked norecurse, we'll mark f() norecurse, which is wrong.
> > >
> > >
> > > We should/do not mark `g` as `norecurse` in this example. At least not if `f` is externally visible.
> >
> >
> > Could you explain a little further? I don't see how changing whether `f` is externally visible changes whether it's OK to mark `g` with `norecurse`. Here's my thinking. If `f` is externally visible that means it can be called from external code, and we have to make the worst-case assumption about the call stack, but in this case we know that `g` can't be on the call stack since `g` doesn't call any external functions. Is that right?
>
>
> I should have explained this better from the beginning. At the moment, for better or worse, we derive `norecurse` only if the function does not call a `recursive` function. I did not want to change this behavior in this patch but I'm willing to investigate if we should. Now, in the example above, `f` is recursive as it calls `g` which calls `f`. And `g` is recursive as it calls `f` which was recursive. My internal comment was following this logic: If `f` was internal, we could (conceptually) eliminate the call of `g` after IPSCCP and thereby making both functions non-recursive.


Ah, through other optimizations. Got it!

>>> What we did before and with this patch is derive `norecurse` only if no call to a recursive function is contained. So in contrast to other attributes we actually do not look at the optimistic/assumed state of called functions.
>> 
>> I think the right fix is to extend it to derive `norecurse` only if no call to a recursive function or a function in the same SCC (even if marked `norecurse`) is contained. That would allow the attributor to avoid marking `f` as `norecurse` even if `g` were marked `norecurse`.
> 
> If we derive what the LangRef says right now, the best solution is `norecurse` for `g` and nothing for `f`. I don't think we need to change the wording. (Maybe I'm missing something here.)

I'm just referring to the words "down any possible call path" in the LangRef because I don't know what to do with that. The sentence starts by saying that `norecurse` means that the function may not call itself, which I think I understand, but then it adds "down any possible call path" and I don't know what that adds, removes, or changes. My best guess is that those words are an implementation detail of the optimizer, because the optimizer looks at all possible call paths when deducing `norecurse`. (For example `noreturn` "indicates that the function never returns normally." If it said "indicates that the function never returns normally down any path" I'd be left wondering what corner case of `noreturn` semantics I'm missing!) I'm suggesting a wording like "This function attribute indicates that the function does not occur twice on the call stack. This produces undefined behaviour if the function ever calls itself directly or indirectly." (LangRef doesn't define what a call stack is, though it uses the notion in the definition of `landingpad` and `catchpad` and I think it's fair to expect readers to be familiar.) Anyways, doesn't need to be changed in this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60074/new/

https://reviews.llvm.org/D60074