[NewGVN] Replace predicate info leftovers

[davide]

Summary

For those watching, we were leaving some ssa.copy around and those arrived to the backend where we failed to lower.
The comment in the code contains the whole story, for those interested :)

[davide]

lib/Transforms/Scalar/NewGVN.cpp

2798–2800

I think I could probably just forcefully cast to CallInst as we're always guaranteed that PredicateInfo will insert calls in the map, FWIW.

[• dberlin]

(You forgot the testcase :P)

lib/Transforms/Scalar/NewGVN.cpp

2784

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.

2795

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

[davide]

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

lib/Transforms/Scalar/NewGVN.cpp

2784

I think the comment can be committed separately (your redacted version). I'll go ahead and do it.

2795

Yes, I like this better too (more surgical)

[davide]

updated, now with testcase.

[• dberlin]

LGTM, thanks!

include/llvm/Transforms/Utils/PredicateInfo.h

     return PredicateMap.lookup(V);
   }
 
+  DenseMap<const Value *, const PredicateBase *> &getPredicateMap() {
+    return PredicateMap;
+  }
+
 protected:
   // Used by PredicateInfo annotater, dumpers, and wrapper pass.
   friend class PredicateInfoAnnotatedWriter;

lib/Transforms/Scalar/NewGVN.cpp

 
   Changed |= eliminateInstructions(F);
 
+  // Replace predicate info `ssa.copy` leftovers with their arguments, and
+  // mark for deletion.
+  // We replace a `ssa.copy` intrinsic in `eliminateInstructions` if the
+  // 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
+  // if we don't find anything using predicate info.
+  // 2) This will generate a congruence class that is either constant or has
+  // at least two members. In the latter case the argument should dominate
+  // all the uses and thus should be used to replace the operand.
+  // In some weird cases where the CFG is strange, 2) doesn't hold, so here
+  // we walk the map of predicate info finding instructions with at least one
+  // use.
+  auto &PredMap = PredInfo->getPredicateMap();
+  for (auto &P : PredMap) {
+    Value *Op = const_cast<Value *>(P.first);
+    auto *I = dyn_cast<Instruction>(Op);
+    if (I && !I->use_empty()) {
+      I->replaceAllUsesWith(I->getOperand(0));
+      markInstructionForDeletion(I);
+    }
+  }
+
   // Delete all instructions marked for deletion.
   for (Instruction *ToErase : InstructionsToErase) {
     if (!ToErase->use_empty())