[PATCH] D29316: Add predicateinfo intrinsic, analysis pass, and basic NewGVN support
Daniel Berlin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 1 20:32:07 PST 2017
dberlin added a comment.
In https://reviews.llvm.org/D29316#664303, @silvas wrote:
> 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.
> 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)
The reason it's not a pass is because passes can't return results from pass.
It could be a utility, right up until we try to decide we want to update it.
ATM, it's fast enough i don't think we have to do that, but it's hard to predict the future.
Truthfully, on the large cfg testcase, most things take 10+ minutes, and we take 600ms.
(dominators takes about the same).
I'm not sure we could really make it faster through updating, we'd just avoid invalidation since most changes are non-destructive.
More information about the llvm-commits