[PATCH] [FunctionAttr] Infer nonnull attributes on returns

[Philip Reames]

Responding to comments.  Any comment not explicitly addressed with be fixed before submission.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:849
@@ +848,3 @@
+                                   bool &Speculative) const {
+  assert(F->getType()->isPointerTy() &&
+         "nonnull only meaningful on pointer types");
----------------
pete wrote:
> All function types are pointer types.  I think you need F->getReturnType()->isPointerTy() as you have below.
Thanks.  Good catch.

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:854
@@ +853,3 @@
+  SmallSetVector<Value *, 8> FlowsToReturn;
+  for (Function::iterator I = F->begin(), E = F->end(); I != E; ++I)
+    if (ReturnInst *Ret = dyn_cast<ReturnInst>(I->getTerminator()))
----------------
pete wrote:
> I think this can just be a foreach loop.
Yep, I copied this from code above.  If you don't mind, I'll leave this as is, then fix both in one patch.

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:858
@@ +857,3 @@
+
+  for (unsigned i = 0; i != FlowsToReturn.size(); ++i) {
+    Value *RetVal = FlowsToReturn[i];
----------------
nlewycky wrote:
> pete wrote:
> > I think this would be better as a worklist you pop values off until its empty.
> Be careful not to break the case of resolving a self recursive phi. I think the normal way to handle this is with a set of "visited" elements, plus a worklist "queue" like Pete suggests.
As Nick pointed out, the use of the SetVector is important here.  Given this is how other algorithms in the same file are structured, I'd prefer not to change this.  

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:887
@@ +886,3 @@
+      PHINode *PN = cast<PHINode>(RVI);
+      for (int i = 0, e = PN->getNumIncomingValues(); i != e; ++i)
+        FlowsToReturn.insert(PN->getIncomingValue(i));
----------------
pete wrote:
> I don't know if there's a foreach on the phi values.  If there is then good to use it, otherwise I guess someone else can add it later.
I copied this from code above.  If you don't mind, I'll leave this as is, then fix both in one patch.

http://reviews.llvm.org/D9688

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/