[IR] Remove the opcode argument from CmpInst::Create

[craig.topper]

Summary

The predicate argument alone should be enough to determine the class that needs to be created.

For some cases where we were always creating an ICmpInst I used the ICmpInst constructor directly. I can do that as a separate patch if that's prefered.

Do I need to be concerned with out of tree users of this?

Not sure who the best reviewers of this are. The CmpInst::Create function has been untouched for many years.

[davide]

This is correct, but why?

[craig.topper]

I think part of another change snuck into this patch. Particularly the stuff in InstCombineSelect.cpp. So ignore that.

I originally noticed this while playing around with removing the one use restriction from this code in visitXor

// not (cmp A, B) = !cmp A, B
CmpInst::Predicate Pred;
if (match(&I, m_Not(m_OneUse(m_Cmp(Pred, m_Value(), m_Value()))))) {
  cast<CmpInst>(Op0)->setPredicate(CmpInst::getInversePredicate(Pred));
  return replaceInstUsesWith(I, Op0);
}

This requires creating a new instruction and we should be able to use CmpInst::Create, but the m_Cmp matcher only returns the Predicate. So we have to examine the predicate to get the correct opcode.

include/llvm/IR/InstrTypes.h

void *operator new(size_t s) {
     return User::operator new(s, 2);
   }
 
   /// Construct a compare instruction, given the opcode, the predicate and
   /// the two operands.  Optionally (if InstBefore is specified) insert the
   /// instruction into a BasicBlock right before the specified instruction.
   /// The specified Instruction is allowed to be a dereferenced end iterator.
   /// @brief Create a CmpInst
-  static CmpInst *Create(OtherOps Op,
-                         Predicate predicate, Value *S1,
+  static CmpInst *Create(Predicate predicate, Value *S1,
                          Value *S2, const Twine &Name = "",
                          Instruction *InsertBefore = nullptr);
 
   /// Construct a compare instruction, given the opcode, the predicate and the
   /// two operands.  Also automatically insert this instruction to the end of
   /// the BasicBlock specified.
   /// @brief Create a CmpInst
-  static CmpInst *Create(OtherOps Op, Predicate predicate, Value *S1,
+  static CmpInst *Create(Predicate predicate, Value *S1,
                          Value *S2, const Twine &Name, BasicBlock *InsertAtEnd);
 
   /// @brief Get the opcode casted to the right type
   OtherOps getOpcode() const {
     return static_cast<OtherOps>(Instruction::getOpcode());
   }
 
   /// @brief Return the predicate for this instruction.

lib/CodeGen/CodeGenPrepare.cpp

for (Value::user_iterator UI = CI->user_begin(), E = CI->user_end();
 
     // If we have already inserted a cmp into this block, use it.
     CmpInst *&InsertedCmp = InsertedCmps[UserBB];
 
     if (!InsertedCmp) {
       BasicBlock::iterator InsertPt = UserBB->getFirstInsertionPt();
       assert(InsertPt != UserBB->end());
       InsertedCmp =
-          CmpInst::Create(CI->getOpcode(), CI->getPredicate(),
+          CmpInst::Create(CI->getPredicate(),
                           CI->getOperand(0), CI->getOperand(1), "", &*InsertPt);
       // Propagate the debug info.
       InsertedCmp->setDebugLoc(CI->getDebugLoc());
     }
 
     // Replace a use of the cmp with a use of the new cmp.
     TheUse = InsertedCmp;
     MadeChange = true;

lib/IR/Constants.cpp

case Instruction::GetElementPtr: {
     if (GO->isInBounds())
       return GetElementPtrInst::CreateInBounds(GO->getSourceElementType(),
                                                Ops[0], Ops.slice(1));
     return GetElementPtrInst::Create(GO->getSourceElementType(), Ops[0],
                                      Ops.slice(1));
   }
   case Instruction::ICmp:
   case Instruction::FCmp:
-    return CmpInst::Create((Instruction::OtherOps)getOpcode(),
-                           (CmpInst::Predicate)getPredicate(), Ops[0], Ops[1]);
+    return CmpInst::Create((CmpInst::Predicate)getPredicate(), Ops[0], Ops[1]);
 
   default:
     assert(getNumOperands() == 2 && "Must be binary operator?");
     BinaryOperator *BO =
       BinaryOperator::Create((Instruction::BinaryOps)getOpcode(),
                              Ops[0], Ops[1]);
     if (isa<OverflowingBinaryOperator>(BO)) {
       BO->setHasNoUnsignedWrap(SubclassOptionalData &

lib/IR/Instructions.cpp

: Instruction(ty, op,
                 InsertAtEnd) {
   Op<0>() = LHS;
   Op<1>() = RHS;
   setPredicate((Predicate)predicate);
   setName(Name);
 }
 
 CmpInst *
-CmpInst::Create(OtherOps Op, Predicate predicate, Value *S1, Value *S2,
+CmpInst::Create(Predicate predicate, Value *S1, Value *S2,
                 const Twine &Name, Instruction *InsertBefore) {
-  if (Op == Instruction::ICmp) {
+  if (isIntPredicate(predicate)) {
     if (InsertBefore)
       return new ICmpInst(InsertBefore, CmpInst::Predicate(predicate),
                           S1, S2, Name);
     else
       return new ICmpInst(CmpInst::Predicate(predicate),
                           S1, S2, Name);
   }
   
   if (InsertBefore)
     return new FCmpInst(InsertBefore, CmpInst::Predicate(predicate),
                         S1, S2, Name);
   else
     return new FCmpInst(CmpInst::Predicate(predicate),
                         S1, S2, Name);
 }
 
 CmpInst *
-CmpInst::Create(OtherOps Op, Predicate predicate, Value *S1, Value *S2,
+CmpInst::Create(Predicate predicate, Value *S1, Value *S2,
                 const Twine &Name, BasicBlock *InsertAtEnd) {
-  if (Op == Instruction::ICmp) {
+  if (isIntPredicate(predicate)) {
     return new ICmpInst(*InsertAtEnd, CmpInst::Predicate(predicate),
                         S1, S2, Name);
   }
   return new FCmpInst(*InsertAtEnd, CmpInst::Predicate(predicate),
                       S1, S2, Name);
 }
 
 void CmpInst::swapOperands() {

lib/Transforms/InstCombine/InstCombineCompares.cpp

Instruction *InstCombiner::visitICmpInst(ICmpInst &I) {
   // comparing -val or val with non-zero is the same as just comparing val
   // ie, abs(val) != 0 -> val != 0
   if (I.getPredicate() == ICmpInst::ICMP_NE && match(Op1, m_Zero())) {
     Value *Cond, *SelectTrue, *SelectFalse;
     if (match(Op0, m_Select(m_Value(Cond), m_Value(SelectTrue),
                             m_Value(SelectFalse)))) {
       if (Value *V = dyn_castNegVal(SelectTrue)) {
         if (V == SelectFalse)
-          return CmpInst::Create(Instruction::ICmp, I.getPredicate(), V, Op1);
+          return new ICmpInst(I.getPredicate(), V, Op1);
       }
       else if (Value *V = dyn_castNegVal(SelectFalse)) {
         if (V == SelectTrue)
-          return CmpInst::Create(Instruction::ICmp, I.getPredicate(), V, Op1);
+          return new ICmpInst(I.getPredicate(), V, Op1);
       }
     }
   }
 
   if (Op0->getType()->getScalarType()->isIntegerTy(1))
     if (Instruction *Res = canonicalizeICmpBool(I, *Builder))
       return Res;
 

lib/Transforms/InstCombine/InstCombinePHI.cpp

for (unsigned i = 1, e = PN.getNumIncomingValues(); i != e; ++i) {
       if (NewRHS) {
         Value *NewInRHS = InInst->getOperand(1);
         NewRHS->addIncoming(NewInRHS, PN.getIncomingBlock(i));
       }
     }
   }
 
   if (CmpInst *CIOp = dyn_cast<CmpInst>(FirstInst)) {
-    CmpInst *NewCI = CmpInst::Create(CIOp->getOpcode(), CIOp->getPredicate(),
-                                     LHSVal, RHSVal);
+    CmpInst *NewCI = CmpInst::Create(CIOp->getPredicate(), LHSVal, RHSVal);
     NewCI->setDebugLoc(PHIArgMergedDebugLoc(PN));
     return NewCI;
   }
 
   BinaryOperator *BinOp = cast<BinaryOperator>(FirstInst);
   BinaryOperator *NewBinOp =
     BinaryOperator::Create(BinOp->getOpcode(), LHSVal, RHSVal);
 
if (BinaryOperator *BinOp = dyn_cast<BinaryOperator>(FirstInst)) {
     for (unsigned i = 1, e = PN.getNumIncomingValues(); i != e; ++i)
       BinOp->andIRFlags(PN.getIncomingValue(i));
 
     BinOp->setDebugLoc(PHIArgMergedDebugLoc(PN));
     return BinOp;
   }
 
   CmpInst *CIOp = cast<CmpInst>(FirstInst);
-  CmpInst *NewCI = CmpInst::Create(CIOp->getOpcode(), CIOp->getPredicate(),
-                                   PhiVal, ConstantOp);
+  CmpInst *NewCI = CmpInst::Create(CIOp->getPredicate(), PhiVal, ConstantOp);
   NewCI->setDebugLoc(PHIArgMergedDebugLoc(PN));
   return NewCI;
 }
 
 /// Return true if this PHI node is only used by a PHI node cycle that is dead.
 static bool DeadPHICycle(PHINode *PN,
                          SmallPtrSetImpl<PHINode*> &PotentiallyDeadPHIs) {
   if (PN->use_empty()) return true;

lib/Transforms/InstCombine/InstCombineSelect.cpp

Instruction *InstCombiner::foldSPFofSPF(Instruction *Inner,
 
   return nullptr;
 }
 
 /// If one of the constants is zero (we know they can't both be) and we have an
 /// icmp instruction with zero, and we have an 'and' with the non-constant value
 /// and a power of two we can turn the select into a shift on the result of the
 /// 'and'.
-static Value *foldSelectICmpAnd(const SelectInst &SI, ConstantInt *TrueVal,
-                                ConstantInt *FalseVal,
+static Value *foldSelectICmpAnd(const SelectInst &SI, APInt TrueVal,
+                                APInt FalseVal,
                                 InstCombiner::BuilderTy *Builder) {
   const ICmpInst *IC = dyn_cast<ICmpInst>(SI.getCondition());
   if (!IC || !IC->isEquality() || !SI.getType()->isIntegerTy())
     return nullptr;
 
   if (!match(IC->getOperand(1), m_Zero()))
     return nullptr;
 
   ConstantInt *AndRHS;
   Value *LHS = IC->getOperand(0);
   if (!match(LHS, m_And(m_Value(), m_ConstantInt(AndRHS))))
     return nullptr;
 
   // If both select arms are non-zero see if we have a select of the form
   // 'x ? 2^n + C : C'. Then we can offset both arms by C, use the logic
   // for 'x ? 2^n : 0' and fix the thing up at the end.
-  ConstantInt *Offset = nullptr;
-  if (!TrueVal->isZero() && !FalseVal->isZero()) {
-    if ((TrueVal->getValue() - FalseVal->getValue()).isPowerOf2())
+  APInt Offset(TrueVal.getBitWidth(), 0);
+  if (!TrueVal.isNullValue() && !FalseVal.isNullValue()) {
+    if ((TrueVal - FalseVal).isPowerOf2())
       Offset = FalseVal;
-    else if ((FalseVal->getValue() - TrueVal->getValue()).isPowerOf2())
+    else if ((FalseVal - TrueVal).isPowerOf2())
       Offset = TrueVal;
     else
       return nullptr;
 
     // Adjust TrueVal and FalseVal to the offset.
-    TrueVal = ConstantInt::get(Builder->getContext(),
-                               TrueVal->getValue() - Offset->getValue());
-    FalseVal = ConstantInt::get(Builder->getContext(),
-                                FalseVal->getValue() - Offset->getValue());
+    TrueVal -= Offset;
+    FalseVal -= Offset;
   }
 
   // Make sure the mask in the 'and' and one of the select arms is a power of 2.
   if (!AndRHS->getValue().isPowerOf2() ||
-      (!TrueVal->getValue().isPowerOf2() &&
-       !FalseVal->getValue().isPowerOf2()))
+      (!TrueVal.isPowerOf2() && !FalseVal.isPowerOf2()))
     return nullptr;
 
   // Determine which shift is needed to transform result of the 'and' into the
   // desired result.
-  ConstantInt *ValC = !TrueVal->isZero() ? TrueVal : FalseVal;
-  unsigned ValZeros = ValC->getValue().logBase2();
+  const APInt &ValC = !TrueVal.isNullValue() ? TrueVal : FalseVal;
+  unsigned ValZeros = ValC.logBase2();
   unsigned AndZeros = AndRHS->getValue().logBase2();
 
   // If types don't match we can still convert the select by introducing a zext
   // or a trunc of the 'and'. The trunc case requires that all of the truncated
   // bits are zero, we can figure that out by looking at the 'and' mask.
-  if (AndZeros >= ValC->getBitWidth())
+  if (AndZeros >= ValC.getBitWidth())
     return nullptr;
 
   Value *V = Builder->CreateZExtOrTrunc(LHS, SI.getType());
   if (ValZeros > AndZeros)
     V = Builder->CreateShl(V, ValZeros - AndZeros);
   else if (ValZeros < AndZeros)
     V = Builder->CreateLShr(V, AndZeros - ValZeros);
 
   // Okay, now we know that everything is set up, we just don't know whether we
   // have a icmp_ne or icmp_eq and whether the true or false val is the zero.
-  bool ShouldNotVal = !TrueVal->isZero();
+  bool ShouldNotVal = !TrueVal.isNullValue();
   ShouldNotVal ^= IC->getPredicate() == ICmpInst::ICMP_NE;
   if (ShouldNotVal)
     V = Builder->CreateXor(V, ValC);
 
   // Apply an offset if needed.
-  if (Offset)
-    V = Builder->CreateAdd(V, Offset);
+  if (!Offset.isNullValue())
+    V = Builder->CreateAdd(V, ConstantInt::get(V->getType(), Offset));
   return V;
 }
 
 /// Turn select C, (X + Y), (X - Y) --> (X + (select C, Y, (-Y))).
 /// This is even legal for FP.
 static Instruction *foldAddSubSelect(SelectInst &SI,
                                      InstCombiner::BuilderTy &Builder) {
   Value *CondVal = SI.getCondition();
if (CondVal->getType()->isVectorTy() == SelType->isVectorTy()) {
     if (match(TrueVal, m_Zero()) && match(FalseVal, m_AllOnes())) {
       Value *NotCond = Builder->CreateNot(CondVal, "not." + CondVal->getName());
       return new SExtInst(NotCond, SelType);
     }
   }
 
   if (ConstantInt *TrueValC = dyn_cast<ConstantInt>(TrueVal))
     if (ConstantInt *FalseValC = dyn_cast<ConstantInt>(FalseVal))
-      if (Value *V = foldSelectICmpAnd(SI, TrueValC, FalseValC, Builder))
+      if (Value *V = foldSelectICmpAnd(SI, TrueValC->getValue(),
+                                       FalseValC->getValue(), Builder))
         return replaceInstUsesWith(SI, V);
 
   // See if we are selecting two values based on a comparison of the two values.
   if (FCmpInst *FCI = dyn_cast<FCmpInst>(CondVal)) {
     if (FCI->getOperand(0) == TrueVal && FCI->getOperand(1) == FalseVal) {
       // Transform (X == Y) ? X : Y  -> Y
       if (FCI->getPredicate() == FCmpInst::FCMP_OEQ) {
         // This is not safe in general for floating point:

lib/Transforms/Vectorize/LoopVectorize.cpp

for (auto &InductionEntry : *List) {
     for (BasicBlock *BB : LoopBypassBlocks)
       BCResumeVal->addIncoming(II.getStartValue(), BB);
     OrigPhi->setIncomingValue(BlockIdx, BCResumeVal);
   }
 
   // Add a check in the middle block to see if we have completed
   // all of the iterations in the first vector loop.
   // If (N - N%VF) == N, then we *don't* need to run the remainder.
-  Value *CmpN =
-      CmpInst::Create(Instruction::ICmp, CmpInst::ICMP_EQ, Count,
-                      CountRoundDown, "cmp.n", MiddleBlock->getTerminator());
+  Value *CmpN = new ICmpInst(MiddleBlock->getTerminator(), ICmpInst::ICMP_EQ,
+                             Count, CountRoundDown, "cmp.n");
   ReplaceInstWithInst(MiddleBlock->getTerminator(),
                       BranchInst::Create(ExitBlock, ScalarPH, CmpN));
 
   // Get ready to start creating new instructions into the vectorized body.
   Builder.SetInsertPoint(&*VecBody->getFirstInsertionPt());
 
   // Save the state.
   LoopVectorPreHeader = Lp->getLoopPreheader();

tools/llvm-stress/llvm-stress.cpp

if (fp) {
       (CmpInst::LAST_FCMP_PREDICATE - CmpInst::FIRST_FCMP_PREDICATE) +
        CmpInst::FIRST_FCMP_PREDICATE;
     } else {
       op = getRandom() %
       (CmpInst::LAST_ICMP_PREDICATE - CmpInst::FIRST_ICMP_PREDICATE) +
        CmpInst::FIRST_ICMP_PREDICATE;
     }
 
-    Value *V = CmpInst::Create(fp ? Instruction::FCmp : Instruction::ICmp,
-                               (CmpInst::Predicate)op, Val0, Val1, "Cmp",
+    Value *V = CmpInst::Create((CmpInst::Predicate)op, Val0, Val1, "Cmp",
                                BB->getTerminator());
     return PT->push_back(V);
   }
 };
 
 } // end anonymous namespace
 
 static void FillFunction(Function *F, Random &R) {

unittests/Analysis/ScalarEvolutionTest.cpp

TEST_F(ScalarEvolutionsTest, ExpandPtrTypeSCEV) {
   GetElementPtrInst *Gep0 =
       GetElementPtrInst::Create(I32Ty, Alloca, Ci32, "gep0", Br);
   CastInst *CastA =
       CastInst::CreateBitOrPointerCast(Gep0, I8PtrTy, "bitcast1", Br);
   GetElementPtrInst *Gep1 =
       GetElementPtrInst::Create(I8Ty, CastA, Ci32, "gep1", Br);
   GetElementPtrInst *Gep2 = GetElementPtrInst::Create(
       I8Ty, UndefValue::get(I8PtrTy), Ci32, "gep2", Br);
-  CmpInst *Cmp = CmpInst::Create(Instruction::ICmp, CmpInst::ICMP_ULT,
-                                 UndefValue::get(I8PtrTy), CastA, "cmp", Br);
+  CmpInst *Cmp = new ICmpInst(Br, ICmpInst::ICMP_ULT,
+                              UndefValue::get(I8PtrTy), CastA, "cmp");
   SelectInst *Sel = SelectInst::Create(Cmp, Gep1, Gep2, "select", Br);
   CastInst *CastB =
       CastInst::CreateBitOrPointerCast(Sel, I32PtrTy, "bitcast2", Br);
 
   ScalarEvolution SE = buildSE(*F);
   auto *S = SE.getSCEV(CastB);
   SCEVExpander Exp(SE, M.getDataLayout(), "expander");
   Value *V =
TEST_F(ScalarEvolutionsTest, SCEVZeroExtendExpr) {
   Type *I8Ty = Type::getInt8Ty(Context);
   Type *I8PtrTy = Type::getInt8PtrTy(Context);
   Value *Accum = Constant::getNullValue(I8PtrTy);
   int Iters = 20;
   for (int i = 0; i < Iters; i++) {
     BasicBlock *IncBB = BasicBlock::Create(Context, "for.inc", F, EndBB);
     auto *PN = PHINode::Create(I64Ty, 2, "", CondBB);
     PN->addIncoming(ConstantInt::get(Context, APInt(64, 100)), PrevBB);
-    auto *Cmp = CmpInst::Create(Instruction::ICmp, CmpInst::ICMP_SGT, PN,
-                                ConstantInt::get(Context, APInt(64, 90)), "cmp",
-                                CondBB);
+    auto *Cmp = new ICmpInst(*CondBB, ICmpInst::ICMP_SGT, PN,
+                             ConstantInt::get(Context, APInt(64, 90)), "cmp");
     BasicBlock *NextBB;
     if (i != Iters - 1)
       NextBB = BasicBlock::Create(Context, "for.cond", F, EndBB);
     else
       NextBB = EndBB;
     BranchInst::Create(IncBB, NextBB, Cmp, CondBB);
     auto *Dec = BinaryOperator::CreateNSWAdd(
         PN, ConstantInt::get(Context, APInt(64, -1)), "dec", IncBB);