IR: de-duplicate two CmpInst routines (NFC)

[artagnon]

De-duplicate the functions getSignedPredicate and getUnsignedPredicate, nearly identical versions of which were present in CmpInst and ICmpInst, creating less confusion.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Ramkumar Ramachandra (artagnon)

Changes

De-duplicate the functions getSignedPredicate and getUnsignedPredicate, nearly identical versions of which were present in CmpInst and ICmpInst, creating less confusion.

---

Full diff: https://github.com/llvm/llvm-project/pull/116866.diff

3 Files Affected:

• (modified) llvm/include/llvm/IR/InstrTypes.h (+17-16)
• (modified) llvm/include/llvm/IR/Instructions.h (-24)
• (modified) llvm/lib/IR/Instructions.cpp (+30-46)

diff --git a/llvm/include/llvm/IR/InstrTypes.h b/llvm/include/llvm/IR/InstrTypes.h
index 1c60eae7f2f85b..568761586326de 100644
--- a/llvm/include/llvm/IR/InstrTypes.h
+++ b/llvm/include/llvm/IR/InstrTypes.h
@@ -935,28 +935,29 @@ class CmpInst : public Instruction {
     return isUnsigned(getPredicate());
   }
 
-  /// For example, ULT->SLT, ULE->SLE, UGT->SGT, UGE->SGE, SLT->Failed assert
-  /// @returns the signed version of the unsigned predicate pred.
-  /// return the signed version of a predicate
+  /// For example, EQ->EQ, SLE->SLE, UGT->SGT, etc.
+  /// @returns the predicate that would be the result if the operand were
+  /// regarded as signed. Asserts on FP predicates.
+  /// Static variant.
   static Predicate getSignedPredicate(Predicate pred);
 
-  /// For example, ULT->SLT, ULE->SLE, UGT->SGT, UGE->SGE, SLT->Failed assert
-  /// @returns the signed version of the predicate for this instruction (which
-  /// has to be an unsigned predicate).
-  /// return the signed version of a predicate
-  Predicate getSignedPredicate() {
+  /// For example, EQ->EQ, SLE->SLE, UGT->SGT, etc.
+  /// @returns the predicate that would be the result if the operand were
+  /// regarded as signed. Asserts on FP predicates.
+  Predicate getSignedPredicate() const {
     return getSignedPredicate(getPredicate());
   }
 
-  /// For example, SLT->ULT, SLE->ULE, SGT->UGT, SGE->UGE, ULT->Failed assert
-  /// @returns the unsigned version of the signed 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. Asserts on FP predicates.
+  /// Static variant.
   static Predicate getUnsignedPredicate(Predicate pred);
 
-  /// For example, SLT->ULT, SLE->ULE, SGT->UGT, SGE->UGE, ULT->Failed assert
-  /// @returns the unsigned version of the predicate for this instruction (which
-  /// has to be an signed predicate).
-  /// return the unsigned version of a predicate
-  Predicate getUnsignedPredicate() {
+  /// For example, EQ->EQ, SLE->ULE, UGT->UGT, etc.
+  /// @returns the predicate that would be the result if the operand were
+  /// regarded as unsigned. Asserts on FP predicates.
+  Predicate getUnsignedPredicate() const {
     return getUnsignedPredicate(getPredicate());
   }
 
@@ -968,7 +969,7 @@ class CmpInst : public Instruction {
   /// For example, SLT->ULT, ULT->SLT, SLE->ULE, ULE->SLE, EQ->Failed assert
   /// @returns the unsigned version of the signed predicate pred or
   ///          the signed version of the signed predicate pred.
-  Predicate getFlippedSignednessPredicate() {
+  Predicate getFlippedSignednessPredicate() const {
     return getFlippedSignednessPredicate(getPredicate());
   }
 
diff --git a/llvm/include/llvm/IR/Instructions.h b/llvm/include/llvm/IR/Instructions.h
index 8eea659a00caf3..0e598d00952172 100644
--- a/llvm/include/llvm/IR/Instructions.h
+++ b/llvm/include/llvm/IR/Instructions.h
@@ -1203,30 +1203,6 @@ class ICmpInst: public CmpInst {
 #endif
   }
 
-  /// 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
-  Predicate getSignedPredicate() const {
-    return getSignedPredicate(getPredicate());
-  }
-
-  /// This is a static version that you can use without an instruction.
-  /// Return the signed version of the predicate.
-  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
-  Predicate getUnsignedPredicate() const {
-    return getUnsignedPredicate(getPredicate());
-  }
-
-  /// This is a static version that you can use without an instruction.
-  /// Return the unsigned version of the predicate.
-  static Predicate getUnsignedPredicate(Predicate pred);
-
   void setSameSign(bool B = true) {
     SubclassOptionalData = (SubclassOptionalData & ~SameSign) | (B * SameSign);
   }
diff --git a/llvm/lib/IR/Instructions.cpp b/llvm/lib/IR/Instructions.cpp
index 5b89a27126150a..20d9fd2b07b48d 100644
--- a/llvm/lib/IR/Instructions.cpp
+++ b/llvm/lib/IR/Instructions.cpp
@@ -3573,32 +3573,6 @@ raw_ostream &llvm::operator<<(raw_ostream &OS, CmpInst::Predicate Pred) {
   return OS;
 }
 
-ICmpInst::Predicate ICmpInst::getSignedPredicate(Predicate pred) {
-  switch (pred) {
-    default: llvm_unreachable("Unknown icmp predicate!");
-    case ICMP_EQ: case ICMP_NE:
-    case ICMP_SGT: case ICMP_SLT: case ICMP_SGE: case ICMP_SLE:
-       return pred;
-    case ICMP_UGT: return ICMP_SGT;
-    case ICMP_ULT: return ICMP_SLT;
-    case ICMP_UGE: return ICMP_SGE;
-    case ICMP_ULE: return ICMP_SLE;
-  }
-}
-
-ICmpInst::Predicate ICmpInst::getUnsignedPredicate(Predicate pred) {
-  switch (pred) {
-    default: llvm_unreachable("Unknown icmp predicate!");
-    case ICMP_EQ: case ICMP_NE:
-    case ICMP_UGT: case ICMP_ULT: case ICMP_UGE: case ICMP_ULE:
-       return pred;
-    case ICMP_SGT: return ICMP_UGT;
-    case ICMP_SLT: return ICMP_ULT;
-    case ICMP_SGE: return ICMP_UGE;
-    case ICMP_SLE: return ICMP_ULE;
-  }
-}
-
 CmpInst::Predicate CmpInst::getSwappedPredicate(Predicate pred) {
   switch (pred) {
     default: llvm_unreachable("Unknown cmp predicate!");
@@ -3719,36 +3693,46 @@ CmpInst::Predicate CmpInst::getFlippedStrictnessPredicate(Predicate pred) {
 }
 
 CmpInst::Predicate CmpInst::getSignedPredicate(Predicate pred) {
-  assert(CmpInst::isUnsigned(pred) && "Call only with unsigned predicates!");
-
   switch (pred) {
   default:
     llvm_unreachable("Unknown predicate!");
-  case CmpInst::ICMP_ULT:
-    return CmpInst::ICMP_SLT;
-  case CmpInst::ICMP_ULE:
-    return CmpInst::ICMP_SLE;
-  case CmpInst::ICMP_UGT:
-    return CmpInst::ICMP_SGT;
-  case CmpInst::ICMP_UGE:
-    return CmpInst::ICMP_SGE;
+  case ICMP_EQ:
+  case ICMP_NE:
+  case ICMP_SGT:
+  case ICMP_SLT:
+  case ICMP_SGE:
+  case ICMP_SLE:
+    return pred;
+  case ICMP_UGT:
+    return ICMP_SGT;
+  case ICMP_ULT:
+    return ICMP_SLT;
+  case ICMP_UGE:
+    return ICMP_SGE;
+  case ICMP_ULE:
+    return ICMP_SLE;
   }
 }
 
 CmpInst::Predicate CmpInst::getUnsignedPredicate(Predicate pred) {
-  assert(CmpInst::isSigned(pred) && "Call only with signed predicates!");
-
   switch (pred) {
   default:
     llvm_unreachable("Unknown predicate!");
-  case CmpInst::ICMP_SLT:
-    return CmpInst::ICMP_ULT;
-  case CmpInst::ICMP_SLE:
-    return CmpInst::ICMP_ULE;
-  case CmpInst::ICMP_SGT:
-    return CmpInst::ICMP_UGT;
-  case CmpInst::ICMP_SGE:
-    return CmpInst::ICMP_UGE;
+  case ICMP_EQ:
+  case ICMP_NE:
+  case ICMP_UGT:
+  case ICMP_ULT:
+  case ICMP_UGE:
+  case ICMP_ULE:
+    return pred;
+  case ICMP_SGT:
+    return ICMP_UGT;
+  case ICMP_SLT:
+    return ICMP_ULT;
+  case ICMP_SGE:
+    return ICMP_UGE;
+  case ICMP_SLE:
+    return ICMP_ULE;
   }
 }
 

[nikic]

I think it might make more sense to drop the methods on CmpInst instead? getSignedPredicate and getUnsignedPredicate only make sense for icmps, so I'd expect them to be on ICmpInst only. Or does that make usage awkward somehow?

[artagnon]

I think it might make more sense to drop the methods on CmpInst instead? getSignedPredicate and getUnsignedPredicate only make sense for icmps, so I'd expect them to be on ICmpInst only. Or does that make usage awkward somehow?

I think we'd need extra downcasts in many places if we move them to ICmpInst. Besides, there are several functions that are specific to FCmpInst that are also in CmpInst, so I don't see the harm in having these here, and asserting appropriately.

[artagnon]

I think it might make more sense to drop the methods on CmpInst instead? getSignedPredicate and getUnsignedPredicate only make sense for icmps, so I'd expect them to be on ICmpInst only. Or does that make usage awkward somehow?

I think we'd need extra downcasts in many places if we move them to ICmpInst. Besides, there are several functions that are specific to FCmpInst that are also in CmpInst, so I don't see the harm in having these here, and asserting appropriately.

I'm happy to report that the code changes were quite minimal.

[nikic]

LGTM