[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 ↗

(On Diff #99093)

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 ↗

(On Diff #99093)

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 ↗

(On Diff #99093)

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 ↗

(On Diff #99093)

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

2795 ↗

(On Diff #99093)

Yes, I like this better too (more surgical)

[davide]

updated, now with testcase.

[• dberlin]

LGTM, thanks!

llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp

if (alwaysAvailable(Leader)) {
 
           // If we get to this point, and the stack is empty we must have a use
           // with nothing we can use to eliminate this use, so just skip it.
           if (EliminationStack.empty())
             continue;
 
           Value *DominatingLeader = EliminationStack.back();
 
+          auto *II = dyn_cast<IntrinsicInst>(DominatingLeader);
+          if (II && II->getIntrinsicID() == Intrinsic::ssa_copy)
+            DominatingLeader = II->getOperand(0);
+
           // Don't replace our existing users with ourselves.
           if (U->get() == DominatingLeader)
             continue;
           DEBUG(dbgs() << "Found replacement " << *DominatingLeader << " for "
                        << *U->get() << " in " << *(U->getUser()) << "\n");
 
           // If we replaced something in an instruction, handle the patching of
           // metadata.  Skip this if we are replacing predicateinfo with its

llvm/trunk/test/Transforms/NewGVN/pr32945.ll

+; RUN: opt -S -newgvn %s | FileCheck %s
+; CHECK-NOT: call i32 @llvm.ssa.copy
+
+@d = external global i32
+@e = external global i32
+define void @tinkywinky() {
+  br i1 true, label %lor.lhs.false, label %cond.true
+lor.lhs.false:
+  %tmp = load i32, i32* @d, align 4
+  %patatino = load i32, i32* null, align 4
+  %or = or i32 %tmp, %patatino
+  store i32 %or, i32* @d, align 4
+  br label %cond.true
+cond.true:
+  %tmp1 = load i32, i32* @e, align 4
+  %tmp2 = load i32, i32* @d, align 4
+  %cmp = icmp eq i32 %tmp1, %tmp2
+  br i1 %cmp, label %cond.true6, label %cond.false
+cond.true6:
+  %cmp7 = icmp slt i32 %tmp1, 0
+  br i1 %cmp7, label %cond.false, label %cond.false
+cond.false:
+  ret void
+}