CmpPredicate: address review

artagnon · artagnon · commit d017cfc4f140 · 2024-11-29T15:08:22.000Z
diff --git a/llvm/include/llvm/IR/CmpPredicate.h b/llvm/include/llvm/IR/CmpPredicate.h
@@ -17,9 +17,9 @@
 
 namespace llvm {
 /// An abstraction over a floating-point predicate, and a pack of an integer
-/// predicate with samesign information. Functions in ICmpInst construct and
-/// return this type in place of a Predicate. It is also implictly constructed
-/// with a Predicate, dropping samesign information.
+/// predicate with samesign information. Some functions in ICmpInst construct
+/// and return this type in place of a Predicate. It is also implictly
+/// constructed with a Predicate, dropping samesign information.
 class CmpPredicate {
   CmpInst::Predicate Pred;
   bool HasSameSign;
@@ -30,26 +30,16 @@ class CmpPredicate {
     assert(!HasSameSign || CmpInst::isIntPredicate(Pred));
   }
 
-  inline operator CmpInst::Predicate() const { return Pred; }
+  operator CmpInst::Predicate() const { return Pred; }
 
-  inline bool hasSameSign() const { return HasSameSign; }
+  bool hasSameSign() const { return HasSameSign; }
 
   static std::optional<CmpPredicate> getMatching(CmpPredicate A,
-                                                 CmpPredicate B) {
-    if (A.Pred == B.Pred)
-      return A.HasSameSign == B.HasSameSign ? A : CmpPredicate(A.Pred);
-    if (A.HasSameSign &&
-        A.Pred == CmpInst::getFlippedSignednessPredicate(B.Pred))
-      return B.Pred;
-    if (B.HasSameSign &&
-        B.Pred == CmpInst::getFlippedSignednessPredicate(A.Pred))
-      return A.Pred;
-    return {};
-  }
+                                                 CmpPredicate B);
 
-  inline bool operator==(CmpInst::Predicate P) const { return Pred == P; }
+  bool operator==(CmpInst::Predicate P) const { return Pred == P; }
 
-  inline bool operator==(CmpPredicate) const = delete;
+  bool operator==(CmpPredicate) const = delete;
 };
 } // namespace llvm
 
diff --git a/llvm/include/llvm/IR/InstrTypes.h b/llvm/include/llvm/IR/InstrTypes.h
@@ -728,24 +728,6 @@ class CmpInst : public Instruction {
           InsertPosition InsertBefore = nullptr,
           Instruction *FlagsSource = nullptr);
 
-  /// Return the signed version of the predicate: variant that operates on
-  /// Predicate; used by the corresponding function in ICmpInst, to operate with
-  /// CmpPredicate.
-  static Predicate getSignedPredicate(Predicate Pred);
-
-  /// Return the unsigned version of the predicate: variant that operates on
-  /// Predicate; used by the corresponding function in ICmpInst, to operate with
-  /// CmpPredicate.
-  static Predicate getUnsignedPredicate(Predicate Pred);
-
-  /// Return the unsigned version of the signed predicate pred or the signed
-  /// version of the signed predicate pred: variant that operates on Predicate;
-  /// used by the corresponding function in ICmpInst, to operate with
-  /// CmpPredicate.
-  static Predicate getFlippedSignednessPredicate(Predicate Pred);
-
-  friend class CmpPredicate;
-
 public:
   // allocate space for exactly two operands
   void *operator new(size_t S) { return User::operator new(S, AllocMarker); }
diff --git a/llvm/include/llvm/IR/Instructions.h b/llvm/include/llvm/IR/Instructions.h
@@ -1226,56 +1226,44 @@ class ICmpInst: public CmpInst {
     return {getSwappedPredicate(Pred), Pred.hasSameSign()};
   }
 
-  /// @returns the swapped predicate along with samesign information.
-  CmpPredicate getSwappedCmpPredicate() const {
+  /// @returns the swapped predicate.
+  Predicate getSwappedCmpPredicate() const {
     return getSwappedPredicate(getCmpPredicate());
   }
 
   /// For example, EQ->EQ, SLE->SLE, UGT->SGT, etc.
   /// @returns the predicate that would be the result if the operand were
   /// regarded as signed.
-  /// Return the signed version of the predicate along with samesign
-  /// information.
-  CmpPredicate getSignedPredicate() const {
-    return getSignedPredicate(getCmpPredicate());
+  /// Return the signed version of the predicate.
+  Predicate getSignedPredicate() const {
+    return getSignedPredicate(getPredicate());
   }
 
-  /// Return the signed version of the predicate along with samesign
-  /// information: static variant.
-  static CmpPredicate getSignedPredicate(CmpPredicate Pred) {
-    return {CmpInst::getSignedPredicate(Pred), Pred.hasSameSign()};
-  }
+  /// Return the signed version of the predicate: static variant.
+  static Predicate getSignedPredicate(Predicate Pred);
 
   /// For example, EQ->EQ, SLE->ULE, UGT->UGT, etc.
   /// @returns the predicate that would be the result if the operand were
   /// regarded as unsigned.
-  /// Return the unsigned version of the predicate along with samesign
-  /// information.
-  CmpPredicate getUnsignedPredicate() const {
-    return getUnsignedPredicate(getCmpPredicate());
+  /// Return the unsigned version of the predicate.
+  Predicate getUnsignedPredicate() const {
+    return getUnsignedPredicate(getPredicate());
   }
 
-  /// Return the unsigned version of the predicate along with samesign
-  /// information: static variant.
-  static CmpPredicate getUnsignedPredicate(CmpPredicate Pred) {
-    return {CmpInst::getUnsignedPredicate(Pred), Pred.hasSameSign()};
-  }
+  /// Return the unsigned version of the predicate: static variant.
+  static Predicate getUnsignedPredicate(Predicate Pred);
 
   /// For example, SLT->ULT, ULT->SLT, SLE->ULE, ULE->SLE, EQ->EQ
   /// @returns the unsigned version of the signed predicate pred or
-  ///          the signed version of the signed predicate pred, along with
-  ///          samesign information.
+  ///          the signed version of the signed predicate pred.
   /// Static variant.
-  static CmpPredicate getFlippedSignednessPredicate(CmpPredicate Pred) {
-    return {CmpInst::getFlippedSignednessPredicate(Pred), Pred.hasSameSign()};
-  }
+  static Predicate getFlippedSignednessPredicate(Predicate Pred);
 
   /// For example, SLT->ULT, ULT->SLT, SLE->ULE, ULE->SLE, EQ->EQ
   /// @returns the unsigned version of the signed predicate pred or
-  ///          the signed version of the signed predicate pred, along with
-  ///          samesign information.
-  CmpPredicate getFlippedSignednessPredicate() const {
-    return getFlippedSignednessPredicate(getCmpPredicate());
+  ///          the signed version of the signed predicate pred.
+  Predicate getFlippedSignednessPredicate() const {
+    return getFlippedSignednessPredicate(getPredicate());
   }
 
   void setSameSign(bool B = true) {
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
@@ -9104,7 +9104,7 @@ bool llvm::matchSimpleRecurrence(const BinaryOperator *I, PHINode *&P,
 }
 
 /// Return true if "icmp Pred LHS RHS" is always true.
-static bool isTruePredicate(CmpPredicate Pred, const Value *LHS,
+static bool isTruePredicate(CmpInst::Predicate Pred, const Value *LHS,
                             const Value *RHS) {
   if (ICmpInst::isTrueWhenEqual(Pred) && LHS == RHS)
     return true;
@@ -9186,8 +9186,8 @@ static bool isTruePredicate(CmpPredicate Pred, const Value *LHS,
 /// Return true if "icmp Pred BLHS BRHS" is true whenever "icmp Pred
 /// ALHS ARHS" is true.  Otherwise, return std::nullopt.
 static std::optional<bool>
-isImpliedCondOperands(CmpPredicate Pred, const Value *ALHS, const Value *ARHS,
-                      const Value *BLHS, const Value *BRHS) {
+isImpliedCondOperands(CmpInst::Predicate Pred, const Value *ALHS,
+                      const Value *ARHS, const Value *BLHS, const Value *BRHS) {
   switch (Pred) {
   default:
     return std::nullopt;
@@ -9256,16 +9256,18 @@ static std::optional<bool> isImpliedCondCommonOperandWithCR(
 /// Return true if LHS implies RHS (expanded to its components as "R0 RPred R1")
 /// is true.  Return false if LHS implies RHS is false. Otherwise, return
 /// std::nullopt if we can't infer anything.
-static std::optional<bool>
-isImpliedCondICmps(const ICmpInst *LHS, CmpPredicate RPred, const Value *R0,
-                   const Value *R1, const DataLayout &DL, bool LHSIsTrue) {
+static std::optional<bool> isImpliedCondICmps(const ICmpInst *LHS,
+                                              CmpInst::Predicate RPred,
+                                              const Value *R0, const Value *R1,
+                                              const DataLayout &DL,
+                                              bool LHSIsTrue) {
   Value *L0 = LHS->getOperand(0);
   Value *L1 = LHS->getOperand(1);
 
   // The rest of the logic assumes the LHS condition is true.  If that's not the
   // case, invert the predicate to make it so.
-  CmpPredicate LPred =
-      LHSIsTrue ? LHS->getCmpPredicate() : LHS->getInverseCmpPredicate();
+  CmpInst::Predicate LPred =
+      LHSIsTrue ? LHS->getPredicate() : LHS->getInversePredicate();
 
   // We can have non-canonical operands, so try to normalize any common operand
   // to L0/R0.
@@ -9342,8 +9344,8 @@ isImpliedCondICmps(const ICmpInst *LHS, CmpPredicate RPred, const Value *R0,
       match(L0, m_c_Add(m_Specific(L1), m_Specific(R1))))
     return CmpPredicate::getMatching(LPred, RPred).has_value();
 
-  if (auto P = CmpPredicate::getMatching(LPred, RPred))
-    return isImpliedCondOperands(*P, L0, L1, R0, R1);
+  if (LPred == RPred)
+    return isImpliedCondOperands(LPred, L0, L1, R0, R1);
 
   return std::nullopt;
 }
diff --git a/llvm/lib/IR/Instructions.cpp b/llvm/lib/IR/Instructions.cpp
@@ -3581,7 +3581,7 @@ raw_ostream &llvm::operator<<(raw_ostream &OS, CmpInst::Predicate Pred) {
   return OS;
 }
 
-ICmpInst::Predicate CmpInst::getSignedPredicate(Predicate pred) {
+ICmpInst::Predicate ICmpInst::getSignedPredicate(Predicate pred) {
   switch (pred) {
     default: llvm_unreachable("Unknown icmp predicate!");
     case ICMP_EQ: case ICMP_NE:
@@ -3594,7 +3594,7 @@ ICmpInst::Predicate CmpInst::getSignedPredicate(Predicate pred) {
   }
 }
 
-ICmpInst::Predicate CmpInst::getUnsignedPredicate(Predicate pred) {
+ICmpInst::Predicate ICmpInst::getUnsignedPredicate(Predicate pred) {
   switch (pred) {
     default: llvm_unreachable("Unknown icmp predicate!");
     case ICMP_EQ: case ICMP_NE:
@@ -3841,7 +3841,7 @@ std::optional<bool> ICmpInst::compare(const KnownBits &LHS,
   }
 }
 
-CmpInst::Predicate CmpInst::getFlippedSignednessPredicate(Predicate pred) {
+CmpInst::Predicate ICmpInst::getFlippedSignednessPredicate(Predicate pred) {
   if (CmpInst::isEquality(pred))
     return pred;
   if (isSigned(pred))
@@ -3915,6 +3915,22 @@ bool CmpInst::isImpliedFalseByMatchingCmp(Predicate Pred1, Predicate Pred2) {
   return isImpliedTrueByMatchingCmp(Pred1, getInversePredicate(Pred2));
 }
 
+//===----------------------------------------------------------------------===//
+//                       CmpPredicate Implementation
+//===----------------------------------------------------------------------===//
+std::optional<CmpPredicate> CmpPredicate::getMatching(CmpPredicate A,
+                                                      CmpPredicate B) {
+  if (A.Pred == B.Pred)
+    return A.HasSameSign == B.HasSameSign ? A : CmpPredicate(A.Pred);
+  if (A.HasSameSign &&
+      A.Pred == ICmpInst::getFlippedSignednessPredicate(B.Pred))
+    return B.Pred;
+  if (B.HasSameSign &&
+      B.Pred == ICmpInst::getFlippedSignednessPredicate(A.Pred))
+    return A.Pred;
+  return {};
+}
+
 //===----------------------------------------------------------------------===//
 //                        SwitchInst Implementation
 //===----------------------------------------------------------------------===//
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -59,7 +59,7 @@ Value *InstCombinerImpl::insertRangeTest(Value *V, const APInt &Lo,
 
   // V >= Min && V <  Hi --> V <  Hi
   // V <  Min || V >= Hi --> V >= Hi
-  CmpPredicate Pred = Inside ? ICmpInst::ICMP_ULT : ICmpInst::ICMP_UGE;
+  ICmpInst::Predicate Pred = Inside ? ICmpInst::ICMP_ULT : ICmpInst::ICMP_UGE;
   if (isSigned ? Lo.isMinSignedValue() : Lo.isMinValue()) {
     Pred = isSigned ? ICmpInst::getSignedPredicate(Pred) : Pred;
     return Builder.CreateICmp(Pred, V, ConstantInt::get(Ty, Hi));
diff --git a/llvm/unittests/SandboxIR/SandboxIRTest.cpp b/llvm/unittests/SandboxIR/SandboxIRTest.cpp
@@ -5838,17 +5838,8 @@ define void @foo(i32 %i0, i32 %i1) {
     checkCommonPredicates(ICmp, LLVMICmp);
     EXPECT_EQ(ICmp->isSigned(), LLVMICmp->isSigned());
     EXPECT_EQ(ICmp->isUnsigned(), LLVMICmp->isUnsigned());
-    EXPECT_EQ(
-        static_cast<llvm::CmpInst::Predicate>(ICmp->getSignedPredicate()),
-        static_cast<llvm::CmpInst::Predicate>(LLVMICmp->getSignedPredicate()));
-    EXPECT_EQ(ICmp->getSignedPredicate().hasSameSign(),
-              LLVMICmp->getSignedPredicate().hasSameSign());
-    EXPECT_EQ(
-        static_cast<llvm::CmpInst::Predicate>(ICmp->getUnsignedPredicate()),
-        static_cast<llvm::CmpInst::Predicate>(
-            LLVMICmp->getUnsignedPredicate()));
-    EXPECT_EQ(ICmp->getUnsignedPredicate().hasSameSign(),
-              LLVMICmp->getUnsignedPredicate().hasSameSign());
+    EXPECT_EQ(ICmp->getSignedPredicate(), LLVMICmp->getSignedPredicate());
+    EXPECT_EQ(ICmp->getUnsignedPredicate(), LLVMICmp->getUnsignedPredicate());
   }
   auto *NewCmp =
       sandboxir::CmpInst::create(llvm::CmpInst::ICMP_ULE, F.getArg(0),
