[PATCH] [FunctionAttr] Infer nonnull attributes on returns

[Pete Cooper]

Hi Philip

I made a few comments, but mostly just about using foreach and some comments.  Nothing major.

Otherwise, LGTM.

Cheers,
Pete


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:73
@@ -69,1 +72,3 @@
 
+    /// Does this function return null?  
+    bool ReturnsNonNull(Function *F, SmallPtrSet<Function*, 8> &,
----------------
I think you need \brief here

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:77
@@ +76,3 @@
+
+    // AddNonNullAttrs - Deduce nonnull attributes for the SCC.
+    bool AddNonNullAttrs(const CallGraphSCC &SCC);
----------------
And here, with \\\ and without listing the function name.

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

================
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()))
----------------
I think this can just be a foreach loop.

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:858
@@ +857,3 @@
+
+  for (unsigned i = 0; i != FlowsToReturn.size(); ++i) {
+    Value *RetVal = FlowsToReturn[i];
----------------
I think this would be better as a worklist you pop values off until its empty.

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:866
@@ +865,3 @@
+    // It wasn't nonnull and we can't look through it..
+    if (!isa<Instruction>(RetVal))
+      return false;
----------------
I think this can be a dyn_cast so that you can fold it with the cast<> below.

================
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));
----------------
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.

http://reviews.llvm.org/D9688

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