[PATCH] D63604: [Attributor] Deduce "nonnull" attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I think this is almost ready.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:261
+  /// all call sites are known, hence the function has internal linkage.
+
+  bool checkForAllCallSites(Function &F, std::function<bool(CallSite)> &Pred,
----------------
No newline and "sites" in the first line.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:682
+
+  /// Generate a function to check whether the value is known as nonnull.
+  /// The generated function returns true if a value satisfies any of
----------------
"Generate a predicate that checks if a given value is assumed nonnull".


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:705
+
+    // We can deduce nonnull if all return value has nonnull attribute.
+
----------------
This comment is a bit out of place. I don't think you need it at all.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:835
+    return ImmutableCallSite(&getAnchoredValue()).getArgument(ArgNo);
+  }
+
----------------
You can pass this value to the `AbstractAttribute` constructor (via a new `AANonNullImpl` and `AANonNull` constructor so there is no need to overload this method and keep track of the argument number.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:854
+  Value &V = *getAssociatedValue();
+
+  auto *AANonNull = A.getAAFor<AANonNullImpl>(*this, V);
----------------
`isKnowNonNull(&V, ...)` ?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1064
 
+bool Attributor::checkForAllCallSites(Function &F,
+                                      std::function<bool(CallSite)> &Pred,
----------------
Could you move this to the remaining Attributor impl, please.


================
Comment at: llvm/test/Transforms/FunctionAttrs/nonnull.ll:344
+; ATTRIBUTOR-NEXT:    unreachable
+ 
 entry:
----------------
These are a lot of copies. Could we somehow make the "default" check and only specialize/copy when the two (-funcattr and -attributor) generate different code? Or is this already what you did and I just missed it? Also, could you add FIXMEs where we fail to deduce something with the attributor?


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

https://reviews.llvm.org/D63604