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

[Philip Reames]

Responding to Chandler's questions.


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

================
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;
+  }
----------------
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?

http://reviews.llvm.org/D9129

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