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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D63046#1535452 <https://reviews.llvm.org/D63046#1535452>, @uenoku wrote:

> > Can you clang format the code?
>
> I always clang format before submitting a patch.
>  Is there something wrong?


I forgot why I thought it was not formatted. Forget that comment.

Last comments wrt class placement.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1037
+
+struct AAWillReturnFunction : AbstractAttribute, BooleanState {
+
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1045
+
+  /// Return true if "willreturn" is known.
+  bool isAssumedWillReturn() const { return getAssumed(); }
----------------
assumed and known mixed up in the comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1056
+  virtual const std::string getAsStr() const override {
+    return getAssumed() ? "will-return" : "may-noreturn";
+  }
----------------
I think it might make sense to use the same spelling in the assumed case as we use for the attr.


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

https://reviews.llvm.org/D63046