[PATCH] Top-Down FunctionAttrs propagation for noalias, dereferenceable and nonnull inference

[Philip Reames]

Overall, looks pretty reasonable.  A few minor code comments inline.

I might suggest separating the addition of the patch and adding to the pass manager.  In particular, adding the second run of the attribute analysis should probably be discussed more broadly.

================
Comment at: lib/Transforms/IPO/FunctionAttrsTD.cpp:82
@@ +81,3 @@
+  bool MadeChange = false;
+  if (F.isDeclaration() || !F.hasLocalLinkage() || F.arg_empty() ||
+      F.use_empty())
----------------
Stylistically, I'd prefer this separated into two checks.  Your combining correctness and profitability criteria.  Also, rather than "return MadeChange", just "return false" for the early exits.  It makes the flow clearer.  

================
Comment at: lib/Transforms/IPO/FunctionAttrsTD.cpp:91
@@ +90,3 @@
+    bool IsAlign;
+    uint64_t MaxSize, MaxAlign;
+
----------------
Please rename MaxSize.  It appears to actually be MaxKnownDeferenceable and the current name is highly confusing.  

================
Comment at: lib/Transforms/IPO/FunctionAttrsTD.cpp:112
@@ +111,3 @@
+  SmallVector<AttrState, 16> ArgumentState;
+  ArgumentState.resize(F.arg_size());
+
----------------
A comment here that reminds the reader the default state is fully speculative/optimistic would be helpful.  

================
Comment at: lib/Transforms/IPO/FunctionAttrsTD.cpp:114
@@ +113,3 @@
+
+  for (Use &U : F.uses()) {
+    User *UR = U.getUser();
----------------
Do you need to iterate the uses, or can you iterate the users directly?

================
Comment at: lib/Transforms/IPO/FunctionAttrsTD.cpp:125
@@ +124,3 @@
+    CallSite CS(cast<Instruction>(UR));
+    if (!CS.isCallee(&U))
+      return MadeChange;
----------------
Shouldn't this be F?

================
Comment at: lib/Transforms/IPO/FunctionAttrsTD.cpp:131
@@ +130,3 @@
+    Function::arg_iterator Arg = F.arg_begin();
+    for (unsigned i = 0, e = ArgumentState.size(); i != e; ++i, ++AI, ++Arg) {
+      // If we've already excluded this argument, ignore it.
----------------
A general point: this function is *way* too complex.  Split it into reasonable helper functions.  One arrangement would be:
struct ArgState
updateArgAtCallSite(...) {}
updateForCallSite(...) {}
identifyCallSites(...) {}
addIndicatedAtrributes(...) {}

================
Comment at: lib/Transforms/IPO/FunctionAttrsTD.cpp:157
@@ +156,3 @@
+          ArgumentState[i].IsDereferenceable = false;
+          // FIXME: isSafeToLoadUnconditionally does not understand memset.
+          // FIXME: We can use getObjectSize for most things, but for mallocs
----------------
Is this a correctness bug?  Or a missed optimization?  I can't tell from your FIXME.  

================
Comment at: lib/Transforms/IPO/FunctionAttrsTD.cpp:170
@@ +169,3 @@
+            // trap, so we can use at least that size.
+            Size = std::max(Size, TypeSize);
+          }
----------------
Er, this doesn't look right.  We've verified that loading from the pointer *up to* a certain number of bytes is safe.  We have not said anything beyond that.  

================
Comment at: lib/Transforms/IPO/FunctionAttrsTD.cpp:175
@@ +174,3 @@
+          if (!ArgumentState[i].MaxSize)
+            ArgumentState[i].IsDereferenceable = false;
+        }
----------------
It looks like IsDerefernceable could become a function of MaxSize on the state object?

================
Comment at: lib/Transforms/IPO/FunctionAttrsTD.cpp:184
@@ +183,3 @@
+        unsigned Align = getKnownAlignment(*AI, DL);
+        if (Align > DL->getABITypeAlignment(ETy))
+          ArgumentState[i].MaxAlign =
----------------
I don't understand why you need this check.  Is it safe to record an align attribute less than the ABI alignment?  If not, that seems slightly odd.  

================
Comment at: lib/Transforms/IPO/FunctionAttrsTD.cpp:236
@@ +235,3 @@
+
+    bool HaveCand = false;
+    for (unsigned i = 0, e = ArgumentState.size(); i != e; ++i)
----------------
I'm not clear why this check is needed.  Aren't you only iterating 'number of argument' times anyways?

================
Comment at: lib/Transforms/IPO/FunctionAttrsTD.cpp:249
@@ +248,3 @@
+  for (unsigned i = 0, e = ArgumentState.size(); i != e; ++i, ++AI) {
+    if (!ArgumentState[i].any() || AI->hasInAllocaAttr() || AI->hasByValAttr())
+      continue;
----------------
The first is just an early exit, but what are the second two checks for? Are they functionally required?

http://reviews.llvm.org/D4609