[PATCH] Teach InlineCost to account for a null check which can be folded away

[Chandler Carruth]

FWIW, I've spent a *lot* of time thinking about this patch. I'm slowly becoming more OK with it. I still have some concerns, but thinking a lot more about the nature of a call site and how we're using it is making me more comfortable.

Two minor points below I'd like your thoughts on...


================
Comment at: lib/Analysis/IPA/InlineCost.cpp:525
@@ -516,1 +524,3 @@
 
+bool CallAnalyzer::isKnownNonNullInCallee(Value *V) {
+  // Does the *call site* have the NonNull attribute set on an argument?  We
----------------
What do you think about making this a generic wrapper around CS.paramHasAttr rather than a not-null specific wrapper? We should be careful to *always* use this method to query for attributes, and I'm not sure we do today.

================
Comment at: lib/Analysis/IPA/InlineCost.cpp:537-542
@@ +536,8 @@
+  
+  // Is this an alloca in the caller?
+  if (isAllocaDerivedArg(V)) {
+    // We can actually predict the result of comparisons between an
+    // alloca-derived value and null. Note that this fires regardless of
+    // SROA firing.
+    return true;
+  }
----------------
Actually, why do we need this at all?

Shouldn't we synthesize a non-null argument attribute given an alloca argument? That would then allow us to propagate it to a non-null parameter attribute if *all* calls have it. That would in turn trip all the optimization opportunities that happen when we *can't* inline. Anyways, somewhat separate from this patch. But if you agree that this should be superfluous, perhaps add a FIXME to remove this code when we DTRT?

Oh no, oh no, never mind. We would still miss cases where we don't re-construct argument attributes on call instructions we've inlined into new callers... There is so much work needed to remove this.

http://reviews.llvm.org/D9129

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