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

[Hal Finkel]

----- Original Message -----
> From: "Philip Reames" <listmail at philipreames.com>
> To: hfinkel at anl.gov, chandlerc at gmail.com, nicholas at mxc.ca, listmail at philipreames.com
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Wednesday, September 10, 2014 12:59:48 PM
> Subject: Re: [PATCH] Top-Down FunctionAttrs propagation for noalias, dereferenceable and nonnull inference
> 
> 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.

Thanks for the review!

I also need to revisit the way that the noalias inference is done because it is not quite right in this patch. Just checking alias(AI, BI) == NoAlias is not sufficient: a[i] and a[i+1] don't alias, but obviously pointers based on them could. I can really only do this when we have distinct sets of underlying objects (which makes several other things much simpler -- including the capture checking and the ramifications on the optimization pipeline).

 -Hal

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

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory