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

[jdoerfert]

Summary

A function is "no-return" if we never reach a return instruction.

We use this logic to identify and mark "no-return" functions. This works
when the AAReturnedValues abstract attribute uses AAIsDead (D65243) and
because AAIsDead relies on AANoReturn.

[jdoerfert]

Minor adjustment to prior patches

[jdoerfert]

Minor changes

[xbolva00]

llvm/lib/Transforms/IPO/Attributor.cpp

938

llvm::all_of?

[jdoerfert]

llvm/lib/Transforms/IPO/Attributor.cpp

938

I can change that, it'll probably appear again in other patches for now.

[jdoerfert]

I'll update this one after AAIsDead is available (see D64162).

[jdoerfert]

Simplify the no-return deduction based on liveness and returned values

[sstefan1]

llvm/lib/Transforms/IPO/Attributor.cpp

2191

no-return state?

[jdoerfert]

llvm/lib/Transforms/IPO/Attributor.cpp

2191

Good catch.

[Meinersbur]

llvm/lib/Transforms/IPO/Attributor.cpp

2177

Generally, lambdas should not be a given a type since each lambda has its own, compiler-private type. In this case, a std::function is needed anyway to be passed as an argument of checkForallReturnedValues.

Have you considered changing checkForallReturnedValues to accept an llvm::function_ref such that the std::function overhead is not required?

llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll

2

Unrelated change?

llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll

36

Out of curiosity, where/how does the Attributor determine this is an infinite recursion? Wouldn't a single recursive call be enough?

[jdoerfert]

llvm/lib/Transforms/IPO/Attributor.cpp

2177

I thought about it at some point after the interface was created. I'll look into it eventually but not in this commit.

llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll

2

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.

llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll

36

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.

[Meinersbur]

llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll

2

Maybe add a TODO/FIXME remark about it?

llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll

36

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:

stackoverflow-clang.ll8 KBDownload

)

Would catchoverflow() still get a noreturn attribute?

[Meinersbur]

llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll

83–84

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.

[jdoerfert]

llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll

2

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.

llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll

36

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.

83–84

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.

[Meinersbur]

llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll

83–84

I had a different structure in mind, namely:

while.body:
  br i1 false label %return, label %while.body

(i.e. the return bb is still connected)

However, LoopDeletion might remove it since it is allowed to assume that loops without sideeffects to terminate (as inherited from C/C++). That is, infinite loops without sideeffects are undefined behavior. To avoid this, front-end with different semantics can insert an llvm.sideffect intrinsic.

Since it is undefined behavior, we may indeed assume noreturn. Sorry for the false alert.

PS: For some reason, LoopDeletion has not been tough this when llvm.sideeffect was added in D38336. It still uses ScalarEvolution to ensure that infinite loops are never removed. I guess the second part of D38336 has never landed.

[nikic]

llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll

83–84

Wait, isn't the current consensus that LLVM IR itself does not have a forward-progress requirement? llvm.sideeffect() is a workaround (and a pretty bad one in terms of optimization impact, to the point that likely nobody uses it), but eventually LLVM should handle loops without forward-progress correctly. (The recently introduced willreturn attribute should solve a large part of the problem in practice, because it can replace the incorrect readonly-based reasoning that is currently used in its place.)

[jdoerfert]

llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll

83–84

@nikic I might misunderstand but: I thought the reason we have llvm.sideeffect() is because LLVM-IR does require forward-progress. So, if you have a no-side-effect potentially endless loop, it can be removed because the forward-progress requirement guarantees it cannot be endless. If you want a true endless loop you need a side-effect, thus, llvm.sideffect(). Please correct me if I'm wrong.

(This does not impact the noreturn logic above though and I will fix the invoke issue in AAIsDead now.)

[nikic]

llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll

83–84

Here's how I understand the current state to be, based on https://bugs.llvm.org/show_bug.cgi?id=965 (maybe @efriedma can confirm whether this is right?)

• Forward-progress is a C++ concept. C does not have a forward-progress guarantee, so LLVM needs to support this (and well).
• LLVM IR has no forward-progress guarantee (currently by omission -- it seems like LangRef should make an explicit mention in either direction).
• In practice, infinite loops are not well-supported yet, which is why llvm.sideeffect() exists as a workaround.
• If necessary for good optimization, a new function attribute may be added to opt-in to forward-progress for use by C++ frontends.

[jdoerfert]

More test, small fixes, review comments integrated

[jdoerfert]

Add stackoverflow example, positive and negative

[Meinersbur]

llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll

83–84

Forward-progress is a C++ concept. C does not have a forward-progress guarantee, so LLVM needs to support this (and well).

C11 section 6.8.5:

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 a for statement) its expression-3, may be assumed by the implementation to terminate.[157])

[157] This is intended to allow compiler transformations such as removal of empty loops even when termination cannot be proven.

There are of course other front-ends with different semantics of endless loop, which is why llvm.sideeffect has been added. This seems to be the accepted solution after discussion in D38336.

llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll

70–79

Why the changes that add control flow to existing tests?

llvm/test/Transforms/FunctionAttrs/noreturn_stackoverflow.ll

1 ↗

(On Diff #213199)

Thank you for adding this test.

7 ↗

(On Diff #213199)

Could you add some lines about what is supposed to happen in both overflow tests? Probably not everyone is familiar with Windows SEH.

31 ↗

(On Diff #213199)

My example showed that invoke ?overflow@@YAXXZ can leave through the unwind edge (even with the printf after the recursive call) and then return. Why is this then marked as noreturn?

If we want to allow the compiler to assume that stack overflow do not happen, including not throwing an SEH exception, I'd like to wait for the result of the RFC.

[nikic]

llvm/test/Transforms/FunctionAttrs/fn_noreturn.ll

83–84

Thanks for the comments, looks like I (and the Rust community as a whole) completely misunderstood LLVM's stance on this issue. I will submit a LangRef patch to clarify this.

[jdoerfert]

llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll

70–79

because the test becomes meaningless otherwise, e.g., the instructions with the effect we want to test for (here free) would be dead.

llvm/test/Transforms/FunctionAttrs/noreturn_stackoverflow.ll

7 ↗

(On Diff #213199)

What is supposed to happen (except what is checked below)? I am not familiar with the windows SEH and I'm unsure why this actually matters. As the comment states, we ignore the possibility of a stackoverflow.

31 ↗

(On Diff #213199)

This is noreturn because it unconditionally invokes a function that is noreturn and nounwind and we do not handle other "exceptions".
The case where the invoked function is noreturn but not nounwind is below. There, the caller is neither marked as noreturn nor nounwind.

If we want to allow the compiler to assume that stack overflow do not happen, including not throwing an SEH exception

We already assume that and we already optimize under that assumptions (your test with O3 doesn't feature a stack overflow).

I'd like to wait for the result of the RFC.

I would prefer not to wait.
I'm unsure what you want to see here other than this behavior? Do you want to assume any function can throw SEH exceptions under windows? If that would be the result of a lang ref change we can disable this deduction under windows.

[jdoerfert]

Add more tests for invoke +noreturn [+nounwind] [+async exceptions].
The problematic behavior was fixed in

https://reviews.llvm.org/rG924d2138fce43e9bcca98d06dccad28927d74c26

and is tested now.

[Meinersbur]

LGTM

Thank you for adding the extensive test cases with different EHPersonalities.

[jdoerfert]

Closed by https://reviews.llvm.org/rGe83f303938a5