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

Chandler Carruth chandlerc at gmail.com
Mon Jun 15 17:21:16 PDT 2015

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.



More information about the llvm-commits mailing list