[GVNHoist] add cast to unbreak windows build

[nickdesaulniers]

Summary

Follow up to
commit 831e99fee90e ("[GVNHoist] don't hoist callbr users into the callbr's block")

Looks like MSVC has trouble with llvm::is_contained. Unbreak the build.

Link: https://lab.llvm.org/buildbot/#/builders/127/builds/45021/steps/7/logs/stdio

[hans]

lgtm

[nickdesaulniers]

I also wonder whether

diff --git a/llvm/lib/Transforms/Scalar/GVNHoist.cpp b/llvm/lib/Transforms/Scalar/GVNHoist.cpp
index abe38aa2a357..04f3df961662 100644
--- a/llvm/lib/Transforms/Scalar/GVNHoist.cpp
+++ b/llvm/lib/Transforms/Scalar/GVNHoist.cpp
@@ -810,13 +810,13 @@ void GVNHoist::checkSafety(CHIArgs C, BasicBlock *BB, GVNHoist::InsKind K,
   int NumBBsOnAllPaths = MaxNumberOfBBSInPath;
   const Instruction *T = BB->getTerminator();
   for (auto CHI : C) {
-    Instruction *Insn = CHI.I;
+    const Instruction *Insn = CHI.I;
     if (!Insn) // No instruction was inserted in this CHI.
       continue;
     // If the Terminator is some kind of "exotic terminator" that produces a
     // value (such as InvokeInst, CallBrInst, or CatchSwitchInst) which the CHI
     // uses, it is not safe to hoist the use above the def.
-    if (!T->use_empty() && is_contained(Insn->operands(), cast<const Value>(T)))
+    if (!T->use_empty() && is_contained(Insn->operands(), T))
       continue;
     if (K == InsKind::Scalar) {
       if (safeToHoistScalar(BB, Insn->getParent(), NumBBsOnAllPaths))

would have done the trick. Should I file a bug about llvm::is_contained/std::find/mscv19?

[kuhar]

In D145969#4190387, @nickdesaulniers wrote:

I also wonder whether

diff --git a/llvm/lib/Transforms/Scalar/GVNHoist.cpp b/llvm/lib/Transforms/Scalar/GVNHoist.cpp
index abe38aa2a357..04f3df961662 100644
--- a/llvm/lib/Transforms/Scalar/GVNHoist.cpp
+++ b/llvm/lib/Transforms/Scalar/GVNHoist.cpp
@@ -810,13 +810,13 @@ void GVNHoist::checkSafety(CHIArgs C, BasicBlock *BB, GVNHoist::InsKind K,
   int NumBBsOnAllPaths = MaxNumberOfBBSInPath;
   const Instruction *T = BB->getTerminator();
   for (auto CHI : C) {
-    Instruction *Insn = CHI.I;
+    const Instruction *Insn = CHI.I;
     if (!Insn) // No instruction was inserted in this CHI.
       continue;
     // If the Terminator is some kind of "exotic terminator" that produces a
     // value (such as InvokeInst, CallBrInst, or CatchSwitchInst) which the CHI
     // uses, it is not safe to hoist the use above the def.
-    if (!T->use_empty() && is_contained(Insn->operands(), cast<const Value>(T)))
+    if (!T->use_empty() && is_contained(Insn->operands(), T))
       continue;
     if (K == InsKind::Scalar) {
       if (safeToHoistScalar(BB, Insn->getParent(), NumBBsOnAllPaths))

would have done the trick. Should I file a bug about llvm::is_contained/std::find/mscv19?

I'd consider defining a new operator== overload to handle cases like this instead.

[nickdesaulniers]

In D145969#4190388, @kuhar wrote:

I'd consider defining a new operator== overload to handle cases like this instead.

On which class? Use or Instruction?

[kuhar]

In D145969#4190390, @nickdesaulniers wrote:

In D145969#4190388, @kuhar wrote:

I'd consider defining a new operator== overload to handle cases like this instead.

On which class? Use or Instruction?

I'm not 100% sure what existing operator== definitions there are, but I'd think we should be able to handle any two T* and U* that are derived from Value, regardless of their constness. This only makes sense if one is the base of the other. I think this could even be a free function.

llvm/lib/Transforms/Scalar/GVNHoist.cpp

void GVNHoist::checkSafety(CHIArgs C, BasicBlock *BB, GVNHoist::InsKind K,
   const Instruction *T = BB->getTerminator();
   for (auto CHI : C) {
     Instruction *Insn = CHI.I;
     if (!Insn) // No instruction was inserted in this CHI.
       continue;
     // If the Terminator is some kind of "exotic terminator" that produces a
     // value (such as InvokeInst, CallBrInst, or CatchSwitchInst) which the CHI
     // uses, it is not safe to hoist the use above the def.
-    if (!T->use_empty() && is_contained(Insn->operands(), T))
+    if (!T->use_empty() && is_contained(Insn->operands(), cast<const Value>(T)))
       continue;
     if (K == InsKind::Scalar) {
       if (safeToHoistScalar(BB, Insn->getParent(), NumBBsOnAllPaths))
         Safe.push_back(CHI);
     } else {
       if (MemoryUseOrDef *UD = MSSA->getMemoryAccess(Insn))
         if (safeToHoistLdSt(T, Insn, UD, K, NumBBsOnAllPaths))
           Safe.push_back(CHI);