[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/nofree-attributor.ll:70-78
+  %cmp = icmp eq i8* %0, null
+  br i1 %cmp, label %rec, label %call
+call:
   tail call void @free(i8* %0) #1
+  br label %end
+rec:
+  tail call void @free_in_scc1(i8* %0)
----------------
Meinersbur wrote:
> Why the changes that add control flow to existing tests?
because the test becomes meaningless otherwise, e.g., the instructions with the effect we want to test for (here `free`) would be dead.


================
Comment at: llvm/test/Transforms/FunctionAttrs/noreturn_stackoverflow.ll:7
+;
+; This test is also a reminder of how we handle (=ignore) stackoverflow exception handling.
+;
----------------
Meinersbur wrote:
> Could you add some lines about what is supposed to happen in both overflow tests? Probably not everyone is familiar with Windows SEH.
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.


================
Comment at: llvm/test/Transforms/FunctionAttrs/noreturn_stackoverflow.ll:31
+
+; CHECK:       Function Attrs: nofree noreturn nosync nounwind
+; CHECK-NEXT:   @"?catchoverflow@@YAHXZ"()
----------------
Meinersbur wrote:
> 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.
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59978