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

[Hal Finkel]

----- Original Message -----
> From: "Hal Finkel" <hfinkel at anl.gov>
> To: reviews+D4609+public+b1f539b8c63822a5 at reviews.llvm.org
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Wednesday, September 10, 2014 1:13:01 PM
> Subject: Re: [PATCH] Top-Down FunctionAttrs propagation for noalias,	dereferenceable and nonnull inference
> 
> ----- 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).

Actually, I take that back. It does not make the capture checking simpler.

 -Hal

> 
>  -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
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

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