[PATCH] D59978: [Attributor] Deduce the "no-return" attribute for functions

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 3 inline comments as done.
jdoerfert added inline comments.


================
Comment at: llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll:2
 ; RUN: opt -functionattrs -attributor -attributor-disable=false -S < %s | FileCheck %s
-; RUN: opt -functionattrs -attributor -attributor-disable=false -attributor-verify=true -S < %s | FileCheck %s
 ;
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > Meinersbur wrote:
> > > Unrelated change?
> > no.
> > 
> > "Verification" is tricky right now, we haven't found a good system yet. This change will break "verification" but not because it actually does something wrong but because the "verification" is not smart enough. As I said, we do not have a good way of replacing the mechanism yet so I disable it here for now.
> Maybe add a TODO/FIXME remark about it?
There is a remark about the problems with the verifier flag already. Coming up with a proper solution to the verification challenge is on my list.


================
Comment at: llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll:37
 entry:
   %call = call i32 @srec16(i32 %a)
   %call1 = call i32 @srec16(i32 %call)
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > Meinersbur wrote:
> > > Out of curiosity, where/how does the Attributor determine this is an infinite recursion? Wouldn't a single recursive call be enough?
> > A single call is sufficient, when I wrote the test I had something else in mind though.
> > No-return is now deduced here because:
> > - Initially we assume `%call` is `noreturn`,
> > - which causes `%call1` and the rest to be dead (`AAIsDead`),
> > - which causes the return to be never encountered by `AAReturnedValues` (after D65243),
> > - which causes `%call` to stay assumed `noreturn`.
> > So a single call suffices.
> > 
> > The cascade might be a nice test to make sure we do not go down the rabbit hole.
> On windows, you can catch a stackoverflow exception. E.g. in this program:
> ```
> #include <cstdlib>
> #include <cstdio>
> 
> void overflow() {
>   overflow();
> }
> 
> int catchoverflow() {
>   __try  {
>     overflow();
>     printf("Exception NOT caught\n");
>     return 0;
>   } 
>   __except(true) {
>     printf("Exception caught\n");
>     return 1;
>   }
>   return 2;
> }
> 
> int main() {
>   auto result = catchoverflow();
>   printf("Done execution result=%i\n", result);
>   return EXIT_SUCCESS;
> }
> ```
> results in (compiled with either msvc or clang-cl):
> ```
> Exception caught
> Done execution result=1
> ```
> The LLVM-IR for this contains:
> ```
> entry:
>   %retval = alloca i32, align 4
>   %__exception_code = alloca i32, align 4
>   invoke void @"?overflow@@YAXXZ"() #6
>           to label %invoke.cont unwind label %catch.dispatch
> 
> catch.dispatch:                                   ; preds = %invoke.cont, %entry
>   %0 = catchswitch within none [label %__except] unwind to caller
> ```
> (entire file: {F9712200})
> 
> Would `catchoverflow()` still get a `noreturn` attribute?
As we discussed in person, stack overflows are currently not part of the LLVM-IR model. I add tests for this and fix the `AAIsDead` handling of `invoke`. I will also improve the langref in the next version.


================
Comment at: llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll:85
 ;
-; FIXME: no-return missing
+; FIXME: no-return missing (D65243 should fix this)
 ; CHECK: Function Attrs: nofree noinline norecurse nosync nounwind readnone uwtable
----------------
Meinersbur wrote:
> I think this loop should not be `noreturn`. The loop deletion pass will remove this loop (due to not having side-effects), simplifying it to
> ```
> entry:
>   ret i32 %a
> ```
> which obviously returns.
1) I think `noreturn` is fine, and loop deletion will not, better should not, delete the loop because it is not sound.

2) Assuming it did for a moment, we would not execute `ret i32 %a` but `unreachable`. There is no way to connect the `return` block to the loop. Assuming there was, what happens if we would have a side effect after the loop:
```
define i32 @dead_return(i32 %a) #0 {
entry:
  br label %while.body

while.body:                                       ; preds = %entry, %while.body
  br label %while.body

return:                                           ; No predecessors!
  call void foo();
  ret i32 %a
}
```
Now, depending on the pass order, the call would be removed (it is not reachable from the entry) or executed, if we assume the loop would actually be deleted and the unconnected block would be executed.

3) The situation where we delete an "endless" loops is
```
define i32 @conditionally_dead_return(i32 %a, i1 %const_cnd) #0 {
entry:
  br label %while.body

while.body:                                       ; preds = %entry, %while.body
  br i1 %const_cnd, label %return, label %while.body

return:                                           ; preds = %while.body
  call void foo();
  ret i32 %a
}
```
Now the loop is conditionally endless and still without side-effect. We cannot mark it as `noreturn` but we can actually delete the loop as we require forward progress. That means in the original example above we can remove the loop and put `unreachable` there as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59978