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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D29316#661376, @sanjoy wrote:

> > Just moving them would not actually invalidate them (since they have all the info in them necessary to give correct answers)
>
> Wouldn't speculating them be a problem?  That is:
>
>   int x = ...;
>   if (0 s< x s< 20) {
>     x_ = predicate_info(x) // attached icmp 0 s< x s< 20
>     r0 = 1 / (x_ + 1);
>     print r0;
>   }
>
>
> to
>
>   int x = ...;
>   x_ = predicate_info(x) // attached icmp 0 s< x s< 20
>   if (0 s< x s< 20) {
>     r0 = 1 / (x_ + 1);
>     print r0;
>   }
>
>
>


It actually knows it belongs inside the if block, it stores the branch block and successor block it belonged to in the info.  They are placed at very specific points, so it's also possible to make the retrieval function verify they have not been moved before returning info (IE it can choose to either not give an answer, or determine the answer is still valid).  We could do this in passes that may move them.

(Probably not return an answer).

So in your above case, if you did this, we would say nothing, and it becomes equivalent to what it was before.

The bigger problem right now is that nothing is looking through the returned intrinsic, but i can easily fix that in any pass we add predicateinfo to.

> Generally, (as I've mentioned on IRC) the only concern I had with this was the compile time hit we may have because of the extra dereferences that will now be necessary to go from a use to its "true" operand.

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.

3. They try to compute the info lazily to avoid the problems of #1 and #2.  This is expensive in a different way.
4. They do nothing, and get worse results (SCCP takes this approach)

The reason, btw, they have to do #1 and #2 at the same time is to handle simple cases like this:

  define i32 @test1(i32 %x) {
      %cmp = icmp eq i32 %x, 50
      br i1 %cmp, label %true, label %false
  true:
      ret i32 %x
  false:
      ret i32 1
  }

%x has a different value in the true block, but has no name for that value.  So either they have to do  eager replacement, or analysis and then separate lookups for each use + general instruction rewriting :(
They all choose the former because the latter is expensive.
With predicateinfo, there is a new name there, so you don't have to do either.
The main cases predicateinfo doesn't get are critical edge cases (which are fixable)

If we can cleanup a bunch of the existing approaches without compile time loss, it's IMHO a win even if we compute it a few times and don't update it.


https://reviews.llvm.org/D29316