[PATCH] D63046: [Attributor] Deduce "willreturn" function attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

@nicholas If you want to discuss any of this further, please say so.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1037
+
+struct AAWillReturnFunction : AbstractAttribute, BooleanState {
+
----------------
jdoerfert wrote:
> jdoerfert wrote:
> > Please create an AAWillReturn interface in the header so it is easier to query the attribute.
> Now I would split this again into:
>  `struct AAWillReturn : public AbstractAttribute ` that lives in the header
> and
>  `struct AAWillReturnImpl : public AAWillReturn, BooleanState` that lives here.
> The idea is that the `AAWillReturn` is exposed to everybody and `AAWillReturnImpl` has the code shared between all `AAWillReturn` deductions. It is, for example, reasonable to have a call site deduction at some point.
Could you move this struct to the header file please?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1177
+        indicatePessimisticFixpoint();
+        return ChangeStatus::CHANGED;
+      }
----------------
I'd argue: Either do not check for noreturn at all, which I would have initially thought we do, or do it explicitly for this function but not the callees. That way, the function which is `noreturn` is not `willreturn` and callers will no be able to get `willreturn`. However, once `willreturn` is deduced, `noreturn` should be irrelevant (IMHO), maybe contradicting.


================
Comment at: llvm/test/Transforms/FunctionAttrs/will-return.ll:1
 ; RUN: opt -functionattrs -S < %s | FileCheck %s --check-prefix=FNATTR
 
----------------
since you renamed the attribute you should think about renaming the file as well ;)


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

https://reviews.llvm.org/D63046