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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
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:
> jdoerfert wrote:
> > 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.
> 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 [[ https://llvm.org/docs/LangRef.html#llvm-sideeffect-intrinsic | `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.
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59978