[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:
> nikic wrote:
> > jdoerfert wrote:
> > > nikic wrote:
> > > > 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.)
> > > @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.)
> > 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.
> > 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59978