[PATCH] D29316: Add predicateinfo intrinsic, analysis pass, and basic NewGVN support

[Daniel Berlin via llvm-commits]

On Wed, Feb 1, 2017 at 6:33 PM, Sean Silva via Phabricator <
reviews at reviews.llvm.org> wrote:

> silvas added a reviewer: chandlerc.
> silvas added a comment.
>
> One key issue here is that this analysis mutates the IR, which is rarely
> done in LLVM, and I think we are trying to remove all cases of doing that
> (as Chandler puts it in http://llvm.org/devmtg/2014-
> 04/PDFs/Talks/Passes.pdf slide 17 "Forms a new, sub-set IR, which is
> problematic"). On that point, you probably want to start a discussion on
> LLVMdev (with a link to this review). I anticipate at least Chandler is
> going to have something to say about this w.r.t the new PM, so make sure to
> CC him.
>

 Literally the only reason it's not a pass is that passes can't return
results, as far as i can tell.


> In https://reviews.llvm.org/D29316#663624, @dberlin wrote:
>
> > Sure, which is why, for the moment, i'd probably start by cleaning up
> our existing usage of this type of info where possible, rather than just
> add it in new places.
> >
> > Generally, our passes are based on algorithms that assume a single ssa
> "name" (for those not familiar, these are not names in llvm, but it's
> easier to talk about names and values than Value and values :P) has a
> single value that can be determined across the function.
> >
> > To handle the cases predicateinfo handles (IE where this is not true),
> they generally take one of a few approaches:
> >
> > 1. They eagerly try to discover and propagate this info by replacing
> uses (This is used in part by GVN and EarlyCSE).  This tradeoff makes them
> unsuitable to be used as analysis, and hard to use on any sub-portion of
> the CFG
> > 2. They maintain complex data structures that try to say what the value
> of a given name is in different blocks.  This is often expensive to
> maintain (scoped hash tables, which have to be rebuilt each time due to
> popping scopes), or expensive to do lookups in (log n if it's an interval
> tree of dfs numbers, whereas gvn's findleader takes O(N)). Sometimes, they
> maintain multiple ones of these, in conjunction with #1 (GVN is worse than
> earlycse here). I tried this approach in NewGVN on a branch, and it's ... a
> mess to do as an analysis.
>
>
> I'm glad to hear that your tried this. My understanding is that
> traditionally in LLVM we try to keep things inferred from the IR cached in
> analyses (and only using explicit annotations for things coming in from the
> frontend).


IMHO, this has turned out to be a bad strategy for a number of things, and
a good one for others.
I don't think we should pretend it is the right tradeoff for everything.



> But it seems that what you are saying is that this is really nontrivial to
> do in this case, with the crux being that you need a primitive for creating
> a new SSA name to sparsely maintain the information you need (with the rest
> cached in the analysis). I think it would be better to call the intrinsic
> "new ssa name" or something which focuses on the primitive mechanism it is
> providing, rather than one user (can't think of other users off the top of
> my head though, though I haven't tried very hard). Still, the issue of
> mutating the IR to insert these new SSA names is quite thorny and deserves
> a thread on llvm-dev IMO; this use case seems pretty compelling.
>

Would you feel the same way if i just made it a pass or a utility?

We have plenty of both that are required that mutate the IR.
(like, for example, LCSSA)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170201/956817aa/attachment.html>