[PATCH] Top-Down FunctionAttrs propagation for noalias, dereferenceable and nonnull inference
Hal Finkel
hfinkel at anl.gov
Wed Sep 10 11:19:11 PDT 2014
----- 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
More information about the llvm-commits
mailing list