Skip to content

Conversation

trokhymchuk
Copy link

@trokhymchuk trokhymchuk commented Aug 26, 2025

Fixes #154242

The following folds are supported:

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@trokhymchuk
Copy link
Author

Looks like I also need advice: current implementation relies upon the fact that constant is always second argument to the icmp and I was unable to thwart the canonicalization, because when I used constant in the 'thwart instruction' I just got constant folded result and 'thwarting' did not change the order of operands, and I am unsure how to thwart correctly or should I support reverse order at all.

@nikic
Copy link
Contributor

nikic commented Aug 27, 2025

Looks like I also need advice: current implementation relies upon the fact that constant is always second argument to the icmp and I was unable to thwart the canonicalization, because when I used constant in the 'thwart instruction' I just got constant folded result and 'thwarting' did not change the order of operands, and I am unsure how to thwart correctly or should I support reverse order at all.

Constants for icmp are always on the right, there is no need to test them on the left. Also the thwarting bit in https://llvm.org/docs/InstCombineContributorGuide.html#id8 is actually outdated (no longer needed). I'll update the docs.

@nikic
Copy link
Contributor

nikic commented Aug 27, 2025

#155574

@trokhymchuk trokhymchuk force-pushed the InstCombine_optimize_unneded_float-to-int_cast_when_icmp branch from 15cb772 to b126599 Compare August 27, 2025 19:36
@trokhymchuk
Copy link
Author

Looks like I also need advice: current implementation relies upon the fact that constant is always second argument to the icmp and I was unable to thwart the canonicalization, because when I used constant in the 'thwart instruction' I just got constant folded result and 'thwarting' did not change the order of operands, and I am unsure how to thwart correctly or should I support reverse order at all.

Constants for icmp are always on the right, there is no need to test them on the left. Also the thwarting bit in https://llvm.org/docs/InstCombineContributorGuide.html#id8 is actually outdated (no longer needed). I'll update the docs.

That is neat, thanks

@trokhymchuk trokhymchuk force-pushed the InstCombine_optimize_unneded_float-to-int_cast_when_icmp branch 2 times, most recently from 786878d to c846b83 Compare August 27, 2025 19:55
@trokhymchuk trokhymchuk marked this pull request as ready for review August 27, 2025 20:03
@trokhymchuk trokhymchuk requested a review from nikic as a code owner August 27, 2025 20:03
@llvmbot llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Aug 27, 2025
@llvmbot
Copy link
Member

llvmbot commented Aug 27, 2025

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-pgo

@llvm/pr-subscribers-llvm-transforms

Author: Artem Trokhymchuk (trokhymchuk)

Changes

Fixes #154242

The following folds are supported:


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp (+206)
  • (modified) llvm/test/Transforms/InstCombine/icmp.ll (+183)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 3a8e043038153..b3df684a227bb 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -12,6 +12,7 @@
 
 #include "InstCombineInternal.h"
 #include "llvm/ADT/APFloat.h"
+#include "llvm/ADT/APInt.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/Statistic.h"
@@ -31,6 +32,7 @@
 #include "llvm/Support/KnownBits.h"
 #include "llvm/Transforms/InstCombine/InstCombiner.h"
 #include <bitset>
+#include <utility>
 
 using namespace llvm;
 using namespace PatternMatch;
@@ -7611,6 +7613,208 @@ Instruction *InstCombinerImpl::foldICmpCommutative(CmpPredicate Pred,
   return nullptr;
 }
 
+enum class SignType {
+  Positive,
+  NonPositive,
+  Negative,
+  NonNegative,
+};
+
+/// Check signess of a constant integer or vector of integers
+///
+/// \param C constant to check for signedness
+/// \param SignType the sign type to check against
+///
+/// \return whether constant is signess corresponds with the requesed requested
+/// sign
+static bool checkConstantSignType(const Constant *C, SignType Sign) {
+  auto Check = [Sign](const ConstantInt *CI) {
+    switch (Sign) {
+    case SignType::Positive:
+      return !CI->isNegative() && !CI->isZero();
+    case SignType::NonPositive:
+      return CI->isNegative() || CI->isZero();
+    case SignType::Negative:
+      return CI->isNegative();
+    case SignType::NonNegative:
+      return !CI->isNegative();
+    default:
+      llvm_unreachable("Unknown sign type");
+    }
+  };
+
+  if (auto *CI = dyn_cast<ConstantInt>(C))
+    return Check(CI);
+
+  // Check every element for vector
+  if (auto *CDV = dyn_cast<ConstantDataVector>(C)) {
+    for (std::size_t i{}; i != CDV->getNumElements(); ++i) {
+      auto *CI = dyn_cast<ConstantInt>(CDV->getElementAsConstant(i));
+      if (!CI || !Check(CI))
+        return false;
+    }
+    return true;
+  }
+
+  return false;
+}
+
+/// Cast ConstantInt to an appropriate APFloat instance
+///
+/// \param CI ConstantInt instance to cast
+/// \param FPType floating point type to cast to
+/// \param Addend addend to add to constant before casting
+///
+/// \return pair {cast status, APFloat result}
+static std::pair<APFloatBase::opStatus, APFloat>
+castCIToAPF(const ConstantInt *CI, const Type *FPType, int Addend) {
+  APFloat FVal{FPType->isFloatTy() ? APFloat::IEEEsingle()
+                                   : APFloat::IEEEdouble()};
+  APInt CIAPIntValue{CI->getValue()};
+
+  if (CIAPIntValue.isMaxSignedValue() && Addend > 0)
+    return std::make_pair(APFloatBase::opStatus::opOverflow, FVal);
+
+  if (CIAPIntValue.isMinSignedValue() && Addend < 0)
+    return std::make_pair(APFloatBase::opStatus::opUnderflow, FVal);
+
+  APFloatBase::opStatus Status{FVal.convertFromAPInt(
+      CI->getValue() + Addend, true, APFloat::rmNearestTiesToEven)};
+  return std::make_pair(Status, FVal);
+}
+
+/// Cast ConstantDataVector<ConstantInt> to ConstantVector<ConstantFP>
+///
+/// \param CDV ConstantInt datavector to cast
+/// \param FPType floating point type to cast to
+/// \param Addend addend to add before casting
+/// \param Context context to use
+///
+/// \return result constant
+static Constant *castCIDVToCFPDV(const ConstantDataVector *CDV, Type *FPType,
+                                 int Addend, LLVMContext &Context) {
+  SmallVector<Constant *, 16> Elts;
+  for (std::size_t i{}; i != CDV->getNumElements(); ++i) {
+    ConstantInt *CI = dyn_cast<ConstantInt>(CDV->getElementAsConstant(i));
+    if (!CI)
+      return nullptr;
+
+    auto ConverstionResult = castCIToAPF(CI, FPType, Addend);
+    if (ConverstionResult.first != APFloat::opOK)
+      return nullptr;
+
+    Elts.push_back(ConstantFP::get(FPType, ConverstionResult.second));
+  }
+  return ConstantVector::get(Elts);
+}
+
+/// Cast integral constant (either scalar or vector) to an appropriate vector
+/// one
+///
+/// \param C integral contsant to cast
+/// \param FPType floating point type to cast to
+/// \param Addend addend to add before casting
+///
+/// \return result constant
+static Constant *castIntegralConstantToFloat(const Constant *C, Type *FPType,
+                                             int Addend) {
+  Type *InnerType;
+  if (FPType->isFloatingPointTy())
+    InnerType = FPType;
+  else if (FPType->isVectorTy())
+    InnerType = llvm::cast<llvm::VectorType>(FPType)->getElementType();
+  else
+    return nullptr;
+
+  if (!InnerType || !InnerType->isFloatingPointTy())
+    return nullptr;
+
+  if (const ConstantInt *CI = dyn_cast<ConstantInt>(C)) {
+    auto ConverstionResult = castCIToAPF(CI, FPType, Addend);
+    if (ConverstionResult.first != APFloat::opOK)
+      return nullptr;
+
+    return ConstantFP::get(FPType, ConverstionResult.second);
+  }
+
+  const ConstantDataVector *CDV = dyn_cast<ConstantDataVector>(C);
+  if (!CDV)
+    return nullptr;
+
+  LLVMContext &Ctx = C->getContext();
+
+  return castCIDVToCFPDV(CDV, InnerType, Addend, Ctx);
+}
+
+/// Fold icmp (fptosi %arg) C -> fcmp $arg
+/// Folds:
+///  - icmp sgt %arg <negative> -> fcmp ogt %arg <negative>
+///  - icmp sgt %arg <non-negative> -> fcmp oge %arg (<non-negative> + 1)
+///  - icmp slt %arg <positive> -> fcmp olt %arg <positive>
+///  - icmp slt %arg <non-positive> -> fcmp ole %arg (<non-positive> - 1)
+///
+/// \param ICmp icmp instruction
+/// \param IC InstCombiner isntance
+///
+/// \return folded instruction or nullptr, if failed to combine instructions
+static Instruction *foldICmpFToSIToFCmp(ICmpInst &ICmp, InstCombiner &IC) {
+  if (ICmp.getPredicate() != ICmpInst::ICMP_SGT &&
+      ICmp.getPredicate() != ICmpInst::ICMP_SLT)
+    return nullptr;
+
+  // Expect that canonical form: first argument is fptosi
+  auto *FToSI = dyn_cast<FPToSIInst>(ICmp.getOperand(0));
+  if (!FToSI)
+    return nullptr;
+
+  Value *FloatOp = FToSI->getOperand(0);
+  if (!FloatOp)
+    return nullptr;
+
+  // And the second must be constant
+  Constant *C = dyn_cast<Constant>(ICmp.getOperand(1));
+  if (!C)
+    return nullptr;
+
+  FCmpInst::Predicate FCmpPredicate;
+  Constant *FCmpConstant{};
+
+  switch (ICmp.getPredicate()) {
+  case ICmpInst::ICMP_SGT:
+    if (checkConstantSignType(C, SignType::Negative)) {
+      // icmp sgt %arg <negative> -> fcmp ogt %arg <negative>
+      FCmpPredicate = FCmpInst::FCMP_OGT;
+      FCmpConstant = castIntegralConstantToFloat(C, FloatOp->getType(), 0);
+    } else if (checkConstantSignType(C, SignType::NonNegative)) {
+      // icmp sgt %arg <non-negative> -> fcmp oge %arg (<non-negative> + 1)
+      FCmpPredicate = FCmpInst::FCMP_OGE;
+      FCmpConstant = castIntegralConstantToFloat(C, FloatOp->getType(), 1);
+    }
+    break;
+  case ICmpInst::ICMP_SLT:
+    if (checkConstantSignType(C, SignType::Positive)) {
+      // icmp slt %arg <positive> -> fcmp olt %arg <positive>
+      FCmpPredicate = FCmpInst::FCMP_OLE;
+      FCmpConstant = castIntegralConstantToFloat(C, FloatOp->getType(), 0);
+    } else if (checkConstantSignType(C, SignType::NonPositive)) {
+      // icmp slt %arg <non-positive> -> fcmp ole %arg (<non-positive> - 1)
+      FCmpPredicate = FCmpInst::FCMP_OLT;
+      FCmpConstant = castIntegralConstantToFloat(C, FloatOp->getType(), -1);
+    }
+    break;
+  default:
+    llvm_unreachable("Unknown icmp comparator");
+  }
+  if (!FCmpConstant)
+    return nullptr;
+
+  IRBuilder<> B(&ICmp);
+  Value *New;
+  // fcmp FCmpPredicate %arg C
+  New = B.CreateFCmp(FCmpPredicate, FloatOp, FCmpConstant);
+  return IC.replaceInstUsesWith(ICmp, New);
+}
+
 Instruction *InstCombinerImpl::visitICmpInst(ICmpInst &I) {
   bool Changed = false;
   const SimplifyQuery Q = SQ.getWithInstruction(&I);
@@ -7748,6 +7952,8 @@ Instruction *InstCombinerImpl::visitICmpInst(ICmpInst &I) {
   if (Instruction *Res =
           foldICmpCommutative(I.getSwappedCmpPredicate(), Op1, Op0, I))
     return Res;
+  if (Instruction *Res = foldICmpFToSIToFCmp(I, *this))
+    return Res;
 
   if (I.isCommutative()) {
     if (auto Pair = matchSymmetricPair(I.getOperand(0), I.getOperand(1))) {
diff --git a/llvm/test/Transforms/InstCombine/icmp.ll b/llvm/test/Transforms/InstCombine/icmp.ll
index 0faa7da482ef2..8df13997f1b1f 100644
--- a/llvm/test/Transforms/InstCombine/icmp.ll
+++ b/llvm/test/Transforms/InstCombine/icmp.ll
@@ -6054,3 +6054,186 @@ define i1 @icmp_samesign_logical_or(i32 %In) {
   %V = select i1 %c1, i1 true, i1 %c2
   ret i1 %V
 }
+
+; https://alive2.llvm.org/ce/z/XtQS6H
+define i1 @float_to_int_comparing_constant1_positive1(float %arg0) {
+; CHECK-LABEL: define i1 @float_to_int_comparing_constant1_positive1(
+; CHECK-SAME: float [[ARG0:%.*]]) {
+; CHECK-NEXT:    [[V1:%.*]] = fcmp ogt float [[ARG0]], -1.000000e+00
+; CHECK-NEXT:    ret i1 [[V1]]
+;
+  %v0 = fptosi float %arg0 to i32
+  %v1 = icmp sgt i32 %v0, -1
+  ret i1 %v1
+}
+
+; https://alive2.llvm.org/ce/z/ZycBgc
+define i1 @float_to_int_comparing_constant1_positive2(float %arg0) {
+; CHECK-LABEL: define i1 @float_to_int_comparing_constant1_positive2(
+; CHECK-SAME: float [[ARG0:%.*]]) {
+; CHECK-NEXT:    [[V1:%.*]] = fcmp oge float [[ARG0]], 2.000000e+00
+; CHECK-NEXT:    ret i1 [[V1]]
+;
+  %v0 = fptosi float %arg0 to i32
+  %v1 = icmp sgt i32 %v0, 1
+  ret i1 %v1
+}
+
+; https://alive2.llvm.org/ce/z/5VRWXi
+define i1 @float_to_int_comparing_constant2_positive1(float %arg0) {
+; CHECK-LABEL: define i1 @float_to_int_comparing_constant2_positive1(
+; CHECK-SAME: float [[ARG0:%.*]]) {
+; CHECK-NEXT:    [[V1:%.*]] = fcmp ole float [[ARG0]], 1.000000e+00
+; CHECK-NEXT:    ret i1 [[V1]]
+;
+  %v0 = fptosi float %arg0 to i32
+  %v1 = icmp slt i32 %v0, 1
+  ret i1 %v1
+}
+
+; https://alive2.llvm.org/ce/z/9bejWa
+define i1 @float_to_int_comparing_constant2_positive2(float %arg0) {
+; CHECK-LABEL: define i1 @float_to_int_comparing_constant2_positive2(
+; CHECK-SAME: float [[ARG0:%.*]]) {
+; CHECK-NEXT:    [[V1:%.*]] = fcmp olt float [[ARG0]], -1.000000e+00
+; CHECK-NEXT:    ret i1 [[V1]]
+;
+  %v0 = fptosi float %arg0 to i32
+  %v1 = icmp slt i32 %v0, 0
+  ret i1 %v1
+}
+
+define i1 @float_to_int_comparing_constant1_negative1(float %arg0) {
+; CHECK-LABEL: define i1 @float_to_int_comparing_constant1_negative1(
+; CHECK-SAME: float [[ARG0:%.*]]) {
+; CHECK-NEXT:    ret i1 false
+;
+  %v0 = fptosi float %arg0 to i8
+  %v1 = icmp sgt i8 %v0, 127
+  ret i1 %v1
+}
+
+define i1 @float_to_int_comparing_constant1_negative2(float %arg0) {
+; CHECK-LABEL: define i1 @float_to_int_comparing_constant1_negative2(
+; CHECK-SAME: float [[ARG0:%.*]]) {
+; CHECK-NEXT:    [[V0:%.*]] = fptosi float [[ARG0]] to i8
+; CHECK-NEXT:    [[V1:%.*]] = icmp eq i8 [[V0]], 127
+; CHECK-NEXT:    ret i1 [[V1]]
+;
+  %v0 = fptosi float %arg0 to i8
+  %v1 = icmp sge i8 %v0, 127
+  ret i1 %v1
+}
+
+define i1 @float_to_int_comparing_constant2_negative1(float %arg0) {
+; CHECK-LABEL: define i1 @float_to_int_comparing_constant2_negative1(
+; CHECK-SAME: float [[ARG0:%.*]]) {
+; CHECK-NEXT:    ret i1 false
+;
+  %v0 = fptosi float %arg0 to i8
+  %v1 = icmp slt i8 %v0, -128
+  ret i1 %v1
+}
+
+define i1 @float_to_int_comparing_constant2_negative2(float %arg0) {
+; CHECK-LABEL: define i1 @float_to_int_comparing_constant2_negative2(
+; CHECK-SAME: float [[ARG0:%.*]]) {
+; CHECK-NEXT:    [[V0:%.*]] = fptosi float [[ARG0]] to i8
+; CHECK-NEXT:    [[V1:%.*]] = icmp eq i8 [[V0]], -128
+; CHECK-NEXT:    ret i1 [[V1]]
+;
+  %v0 = fptosi float %arg0 to i8
+  %v1 = icmp sle i8 %v0, -128
+  ret i1 %v1
+}
+
+define <2 x i1> @float_to_int_comparing_constant_vec_positive1(<2 x float> %arg0) {
+; CHECK-LABEL: define <2 x i1> @float_to_int_comparing_constant_vec_positive1(
+; CHECK-SAME: <2 x float> [[ARG0:%.*]]) {
+; CHECK-NEXT:    [[V1:%.*]] = fcmp ogt <2 x float> [[ARG0]], splat (float -1.000000e+00)
+; CHECK-NEXT:    ret <2 x i1> [[V1]]
+;
+  %v0 = fptosi <2 x float> %arg0 to <2 x i32>
+  %v1 = icmp sgt <2 x i32> %v0, <i32 -1, i32 -1>
+  ret <2 x i1> %v1
+}
+
+define <2 x i1> @float_to_int_comparing_constant_vec_positive2(<2 x float> %arg0) {
+; CHECK-LABEL: define <2 x i1> @float_to_int_comparing_constant_vec_positive2(
+; CHECK-SAME: <2 x float> [[ARG0:%.*]]) {
+; CHECK-NEXT:    [[V1:%.*]] = fcmp oge <2 x float> [[ARG0]], <float 1.000000e+00, float 2.000000e+00>
+; CHECK-NEXT:    ret <2 x i1> [[V1]]
+;
+  %v0 = fptosi <2 x float> %arg0 to <2 x i32>
+  %v1 = icmp sgt <2 x i32> %v0, <i32 0, i32 1>
+  ret <2 x i1> %v1
+}
+
+
+define <2 x i1> @float_to_int_comparing_constant_vec_positive3(<2 x float> %arg0) {
+; CHECK-LABEL: define <2 x i1> @float_to_int_comparing_constant_vec_positive3(
+; CHECK-SAME: <2 x float> [[ARG0:%.*]]) {
+; CHECK-NEXT:    [[V1:%.*]] = fcmp ole <2 x float> [[ARG0]], splat (float 1.000000e+00)
+; CHECK-NEXT:    ret <2 x i1> [[V1]]
+;
+  %v0 = fptosi <2 x float> %arg0 to <2 x i32>
+  %v1 = icmp slt <2 x i32> %v0, <i32 1, i32 1>
+  ret <2 x i1> %v1
+}
+
+define <2 x i1> @float_to_int_comparing_constant_vec_positive4(<2 x float> %arg0) {
+; CHECK-LABEL: define <2 x i1> @float_to_int_comparing_constant_vec_positive4(
+; CHECK-SAME: <2 x float> [[ARG0:%.*]]) {
+; CHECK-NEXT:    [[V1:%.*]] = fcmp olt <2 x float> [[ARG0]], <float -2.000000e+00, float -1.000000e+00>
+; CHECK-NEXT:    ret <2 x i1> [[V1]]
+;
+  %v0 = fptosi <2 x float> %arg0 to <2 x i32>
+  %v1 = icmp slt <2 x i32> %v0, <i32 -1, i32 0>
+  ret <2 x i1> %v1
+}
+
+
+define <2 x i1> @float_to_int_comparing_constant_vec_negative1(<2 x float> %arg0) {
+; CHECK-LABEL: define <2 x i1> @float_to_int_comparing_constant_vec_negative1(
+; CHECK-SAME: <2 x float> [[ARG0:%.*]]) {
+; CHECK-NEXT:    [[V0:%.*]] = fptosi <2 x float> [[ARG0]] to <2 x i32>
+; CHECK-NEXT:    [[V1:%.*]] = icmp sgt <2 x i32> [[V0]], <i32 -1, i32 1>
+; CHECK-NEXT:    ret <2 x i1> [[V1]]
+;
+  %v0 = fptosi <2 x float> %arg0 to <2 x i32>
+  %v1 = icmp sgt <2 x i32> %v0, <i32 -1, i32 1>
+  ret <2 x i1> %v1
+}
+
+define <2 x i1> @float_to_int_comparing_constant_vec_negative2(<2 x float> %arg0) {
+; CHECK-LABEL: define <2 x i1> @float_to_int_comparing_constant_vec_negative2(
+; CHECK-SAME: <2 x float> [[ARG0:%.*]]) {
+; CHECK-NEXT:    [[V0:%.*]] = fptosi <2 x float> [[ARG0]] to <2 x i32>
+; CHECK-NEXT:    [[V1:%.*]] = icmp slt <2 x i32> [[V0]], <i32 -1, i32 1>
+; CHECK-NEXT:    ret <2 x i1> [[V1]]
+;
+  %v0 = fptosi <2 x float> %arg0 to <2 x i32>
+  %v1 = icmp slt <2 x i32> %v0, <i32 -1, i32 1>
+  ret <2 x i1> %v1
+}
+
+
+define <2 x i1> @float_to_int_comparing_constant_vec_negative3(<2 x float> %arg0) {
+; CHECK-LABEL: define <2 x i1> @float_to_int_comparing_constant_vec_negative3(
+; CHECK-SAME: <2 x float> [[ARG0:%.*]]) {
+; CHECK-NEXT:    ret <2 x i1> zeroinitializer
+;
+  %v0 = fptosi <2 x float> %arg0 to <2 x i8>
+  %v1 = icmp sgt <2 x i8> %v0, <i8 127, i8 127>
+  ret <2 x i1> %v1
+}
+
+define <2 x i1> @float_to_int_comparing_constant_vec_negative4(<2 x float> %arg0) {
+; CHECK-LABEL: define <2 x i1> @float_to_int_comparing_constant_vec_negative4(
+; CHECK-SAME: <2 x float> [[ARG0:%.*]]) {
+; CHECK-NEXT:    ret <2 x i1> zeroinitializer
+;
+  %v0 = fptosi <2 x float> %arg0 to <2 x i8>
+  %v1 = icmp slt <2 x i8> %v0, <i8 -128, i8 -128>
+  ret <2 x i1> %v1
+}

@dtcxzyw dtcxzyw changed the title Inst combine optimize unneded float to int cast when icmp [InstCombine] Optimize unneeded float to int cast when icmp Aug 30, 2025
@trokhymchuk trokhymchuk force-pushed the InstCombine_optimize_unneded_float-to-int_cast_when_icmp branch 4 times, most recently from 18f25fe to 094d708 Compare August 31, 2025 21:35
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Drive-by comments only)

@trokhymchuk trokhymchuk force-pushed the InstCombine_optimize_unneded_float-to-int_cast_when_icmp branch 2 times, most recently from bcd8781 to 4b89232 Compare September 2, 2025 19:02
@dtcxzyw
Copy link
Member

dtcxzyw commented Sep 4, 2025

@zyw-bot mfuzz

Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miscompilation reproducer: https://alive2.llvm.org/ce/z/3hmRxH

@trokhymchuk trokhymchuk force-pushed the InstCombine_optimize_unneded_float-to-int_cast_when_icmp branch from 4b89232 to d6c8c1b Compare September 7, 2025 01:17
@trokhymchuk
Copy link
Author

Miscompilation reproducer: https://alive2.llvm.org/ce/z/3hmRxH

yep, my bad, messed with comparators and did not check the optimization result ones, fixed now, @dtcxzyw pls take a look

@dtcxzyw
Copy link
Member

dtcxzyw commented Sep 7, 2025

@trokhymchuk Can you please provide a generalized proof? See https://llvm.org/docs/InstCombineContributorGuide.html#proofs.

return nullptr;

FCmpInst::Predicate FCmpPredicate;
Constant *FCmpConstant{};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Constant *FCmpConstant{};
Constant *FCmpConstant;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as below

return nullptr;

if (Pred != ICmpInst::ICMP_SGT && Pred != ICmpInst::ICMP_SLT)
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use default: return nullptr instead of the separate check.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also had this thought (using pointer as boolean marker does not look pretty to me also) the change would also require adding additional else to ifs: if non-splat vector with values [1, -1] it is neither positive, non-positive, negative nor non-negative and thus neither if branch would be executed. nullptr works for that case, but default would not. Additionally, FCmpConstant checked for nullptr works when integer is failed to convert to fp number.

@trokhymchuk trokhymchuk force-pushed the InstCombine_optimize_unneded_float-to-int_cast_when_icmp branch 2 times, most recently from 6627fde to baa4c30 Compare September 7, 2025 11:10
@trokhymchuk
Copy link
Author

@trokhymchuk Can you please provide a generalized proof? See https://llvm.org/docs/InstCombineContributorGuide.html#proofs.

generalized proofs:

@trokhymchuk trokhymchuk force-pushed the InstCombine_optimize_unneded_float-to-int_cast_when_icmp branch from baa4c30 to aea0f5b Compare September 7, 2025 11:36
@dtcxzyw
Copy link
Member

dtcxzyw commented Sep 7, 2025

icmp sgt (fptosi %x), -> fcmp ogt %x, : https://alive2.llvm.org/ce/z/isTzu2

You should use %neg = icmp slt i8 %C, 0.

Copy link

github-actions bot commented Sep 7, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@trokhymchuk
Copy link
Author

icmp sgt (fptosi %x), -> fcmp ogt %x, : https://alive2.llvm.org/ce/z/isTzu2

You should use %neg = icmp slt i8 %C, 0.

yep, thanks, updated the PR description: https://alive2.llvm.org/ce/z/etWGrN

@trokhymchuk trokhymchuk force-pushed the InstCombine_optimize_unneded_float-to-int_cast_when_icmp branch from aea0f5b to 0effb00 Compare September 7, 2025 11:48
…ion tests

Add optimization test reducing unneded float-to-int cast when
comparing numbers:
* icmp sgt (fptosi %x), <negative> -> fcmp ogt %x, <negative>
* icmp sgt (fptosi %x), <non-negative> -> fcmp oge %x, <non-negative + 1>
* icmp slt (fptosi %x), <positive> -> fcmp olt %x, <positive>
* icmp slt (fptosi %x), <non-positive> -> fcmp ole %x, <non-positive - 1>
@trokhymchuk trokhymchuk force-pushed the InstCombine_optimize_unneded_float-to-int_cast_when_icmp branch from 0effb00 to b65aa2d Compare September 7, 2025 12:14
@llvmbot llvmbot added the PGO Profile Guided Optimizations label Sep 7, 2025
@trokhymchuk trokhymchuk requested a review from dtcxzyw September 7, 2025 12:14
@trokhymchuk
Copy link
Author

While running regression tests found problem with Transforms/PGOProfile/chr.ll test, looks like current optimization applies there also, fixed in the separate commit (b65aa2d) (pls comment if the commit must be squashed with previous)

@dtcxzyw
Copy link
Member

dtcxzyw commented Sep 7, 2025

We may need a one-use check on the fptosi cast. This is a bit different from our general policy, as the fp constant materialization is not as cheap as integers.

@trokhymchuk trokhymchuk force-pushed the InstCombine_optimize_unneded_float-to-int_cast_when_icmp branch from b65aa2d to 039754c Compare September 7, 2025 13:12
@trokhymchuk
Copy link
Author

We may need a one-use check on the fptosi cast. This is a bit different from our general policy, as the fp constant materialization is not as cheap as integers.

I added m_OneUse check (https://github.com/llvm/llvm-project/pull/155501/files#diff-45d9bc48ca662c4f70ef6d841aeef9f8cf5465414b844562e41e237888b60d8cR7655) for fptosi, pls take a look

@trokhymchuk trokhymchuk force-pushed the InstCombine_optimize_unneded_float-to-int_cast_when_icmp branch from 039754c to 64e797c Compare September 7, 2025 13:16
@nikic
Copy link
Contributor

nikic commented Sep 7, 2025

It looks like these two have an additional pre-condition on the min/max value, but it doesn't seem to be checked in your implementation?

…ion implementation

Add optimization test reducing unneded float-to-int cast when
comparing numbers:
* icmp sgt (fptosi %x), <negative> -> fcmp ogt %x, <negative>
* icmp sgt (fptosi %x), <non-negative> -> fcmp oge %x, <non-negative + 1>
* icmp slt (fptosi %x), <positive> -> fcmp olt %x, <positive>
* icmp slt (fptosi %x), <non-positive> -> fcmp ole %x, <non-positive - 1>
…ssion tests

Fix regression in the llvm/test/Transforms/PGOProfile/chr.ll after
optimizing fptosi cast in icmp.
@trokhymchuk trokhymchuk force-pushed the InstCombine_optimize_unneded_float-to-int_cast_when_icmp branch from 64e797c to bf38c1b Compare September 7, 2025 14:37
@llvmbot llvmbot added the llvm:ir label Sep 7, 2025
@trokhymchuk
Copy link
Author

It looks like these two have an additional pre-condition on the min/max value, but it doesn't seem to be checked in your implementation?

thsnk for noticing, I added those corner-cases and thought that ConstantFoldBinaryOpOperands would return nullptr, but looks like it would not...

I added m_MaxSignedValue() and m_MinSignedValue() checks, later one was not implemented so I added it also

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:ir llvm:transforms PGO Profile Guided Optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missed optimization: fold icmp sgt (fptosi %x), -1 -> fcmp ogt %x, -1.0
4 participants