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

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

Sorry, I didn't do `git add` of the testcase, I'll do in my next upload.



================
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
----------------
dberlin wrote:
> 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.
> 
> 
I think the comment can be committed separately (your redacted version). I'll go ahead and do it.


================
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);
----------------
dberlin wrote:
> 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
> 
> ```
> 
Yes, I like this better too (more surgical)


https://reviews.llvm.org/D33226