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

[Chandler Carruth]

================
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
----------------
reames wrote:
> chandlerc wrote:
> > 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.
> I'm not sure what the point of this would be.  Are you looking for something to limit access to the CallSite variable?  Or something else?
> 
> Just to make sure I read your question right, you were asking about a wrapper of the form:
> bool CallAnalyzer::hasParamAttr(Value *V, Attributes::AttributeKind Attr)
> 
> Right?
> 
> (I'm not *opposed* to this; I just don't see the point of the extra abstraction.)
The reason for suggesting this is that it makes it obvious how the next person should query some *other* attribute off of the call site in the CallAnalyzer. It also (IMO) makes it less likely that they'll do so incorrectly.

Essentially, I'm trying to separate the non-null property testing from the callsite attribute querying.

I would probably name it 'isArgumentWithAttribute(Value *V, Attributes::AttributeKind Attr)' or something along those lines.

================
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;
+  }
----------------
reames wrote:
> chandlerc wrote:
> > 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.
> I was with you until your last paragraph where you pointed out a case I hadn't thought of.  Just to make sure I'm clear, you concerned about something like this right?
> a() { alloca a; b(a ) }
> b(ptr) { c(ptr) }
> c(ptr) { if ptr != null: do_x() }
> 
> With the scheme you proposed, we'd have:
> inline b into a
> reanalyze b's callsite to c in a
> inline c into a
> 
> It's the need for the reanalyze step your concerned about right?  In particular, whether this could happen within the inliner itself without running additional passes?
Yes, exactly.

http://reviews.llvm.org/D9129

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