[PATCH] D33226: [NewGVN] Replace predicate info leftovers

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

(You forgot the testcase :P)



================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:2784
+  // following two invariants hold:
+  // 1) The `ssa.copy` will be at least to its argument. We guarantee this
+  // by returning a variable consisting of the argument as the value number
----------------
This isn't quite right.
Let me try to rewrite what i wrote in the bug report better here:

In most cases, we are able to eliminate the `ssa.copy` intrinsics in `eliminateInstructions`.
The two cases we are able to eliminate them are:

1. We don't use the predicate info (in which case, we will value number the `ssa.copy` to its argument)
2. We use the predicate info to find an equivalent, and something in the congruence class we come up with dominates the predicateinfo.

This is the vast majority of cases.
However, we are not guaranteed that any of the equivalents we  come up with if we use the predicateinfo dominates it.
This is particularly true when the used predicateinfo is an and/or of two conditions. 

In these cases, we will end up with some `ssa.copy` left after  and need to eliminate them.




================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:2795
+  for (auto &P : PredMap) {
+    Value *Op = const_cast<Value *>(P.first);
+    auto *I = dyn_cast<Instruction>(Op);
----------------
I realized there's an easier way to accomplish this.
In eliminateInstructions, 
where we have:
          // Don't replace our existing users with ourselves.
          if (U->get() == DominatingLeader)
            continue;

right before this, add

```
if (DominatingLeader is a  PredicateInfo copy)
  DominatingLeader = copy argument

```



https://reviews.llvm.org/D33226