[PATCH] D63046: [Attributor] Deduce "willreturn" function attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D63046#1535808 <https://reviews.llvm.org/D63046#1535808>, @nicholas wrote:

> In D63046#1535804 <https://reviews.llvm.org/D63046#1535804>, @nicholas wrote:
>
> > > The current algorithm can deduce only for very simple cases.
> > > 
> > > - A function doesn't have any loop.
> > > - A function doesn't have any recursion.
> >
> > What about functions that exit the program, throw an exception, or longjmp?
>


That was an oversight in the comment but not the code, I think. There are tests for calls and exceptions. The former may prevent `willreturn` the latter doesn't (as of D62801 <https://reviews.llvm.org/D62801>). This way, `willreturn` is orthogonal to `nothrow`.

> I know I just posted, but I think I figured out the right rule. The rule you want is a) doesn't have a cycle b) all calls have `willreturn`. A function with no calls (and no cycles) can have `willreturn`, and a function that calls only `willreturn` functions (and no cycles) can have `willreturn`. That's assuming you don't mind a function being both `willreturn` and `noreturn` at the same time (a function that ends in unreachable). I think we just accept that, there's no point trying to prove any code free from undefined behaviour.
> 
> If I'm right, this patch doesn't need to depend on `norecurse`.

Strictly speaking, it doesn't. But for now, that makes it much simpler. If you only make sure calls are optimistically `willreturn` you get `willreturn` for a potential endless recursion. That is, a function that might, depending on the input, recurse forever or not. Now, `norecurse` is actually "too strong" but that is fine for now.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1115-1120
+    Visited.insert(BB);
+    for (auto *SuccBB : successors(BB)) {
+      if (Visited.count(SuccBB))
+        return true;
+    }
+  }
----------------
nicholas wrote:
> FYI, with effort, this could be pushed to be more efficient. df_iterator maintains its own copy of Visited, and it does its own scan over the successors of the block, neither of which need to be done twice. Unfortunately the fix is not entirely simple.
Let's not optimize it preemptively. I'll run it through LLVM-TS and SPEC2006 before I commit it anyway, if there is a change in compile or runtime I'll report it back.


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

https://reviews.llvm.org/D63046