[PATCH] D27855: try to extend nonnull-ness of arguments from a callsite back to its parent function

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Minor comments inline.  I'm going to defer to Chandler on the clang specific implications here.  This will strictly benefit my frontend and I'd like to see it land.



================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:537
+/// arguments. In general, we don't alter the IR to preserve analysis, but in
+/// this case inlining may cause information loss because the attribute
+/// knowledge may disappear with the call.
----------------
I know this was request earlier in the review thread, but there are lots of cases where inlining drops information available at the call site.   This isn't specific to this case.  Doesn't hurt to call it out, but the wording seems to imply this is unique to this inference which it really isn't.  


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:540
+static bool addArgumentAttrsFromCallsites(Function &F) {
+  bool PropagatedAttr = false;
+
----------------
minor: call this Changed for consistency with the rest of the code.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:547
+  BasicBlock &Entry = F.getEntryBlock();
+  for (Instruction &I : Entry) {
+    if (auto *Call = dyn_cast<CallInst>(&I)) {
----------------
Scanning only the entry block is perfectly reasonable for an initial patch, but could be easily extended, even without dominator information.  walking forwards along single target branches or backwards from return points along single predecessor branches would give a more robust analysis.  

(I would prefer this be done as a separate patch to make it easier to review.)


https://reviews.llvm.org/D27855