[ConstraintElimination] Add support for UCMP/SCMP intrinsics #97974

Poseydon42 · 2024-07-07T22:11:32Z

This adds checks to fold calls to ucmp/scmp where a comparative relationship between the arguments can be established.

llvmbot · 2024-07-07T22:12:04Z

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-transforms

Author: None (Poseydon42)

Changes

This adds checks to fold calls to ucmp/scmp where a comparative relationship between the arguments can be established.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/ConstraintElimination.cpp (+34)
(added) llvm/test/Transforms/ConstraintElimination/uscmp.ll (+110)

diff --git a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
index bf0c67d9dbc4f..478aea7287875 100644
--- a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
@@ -1073,6 +1073,8 @@ void State::addInfoFor(BasicBlock &BB) {
     }
     // Enqueue ssub_with_overflow for simplification.
     case Intrinsic::ssub_with_overflow:
+    case Intrinsic::ucmp:
+    case Intrinsic::scmp:
       WorkList.push_back(
           FactOrCheck::getCheck(DT.getNode(&BB), cast<CallInst>(&I)));
       break;
@@ -1434,6 +1436,33 @@ static bool checkAndReplaceMinMax(MinMaxIntrinsic *MinMax, ConstraintInfo &Info,
   return false;
 }
 
+static bool checkAndReplaceCmp(IntrinsicInst *II, ConstraintInfo &Info,
+                               SmallVectorImpl<Instruction *> &ToRemove) {
+  bool IsSigned = II->getIntrinsicID() == Intrinsic::scmp;
+  Value *LHS = II->getOperand(0);
+  Value *RHS = II->getOperand(1);
+  if (checkCondition(IsSigned ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT, LHS,
+                     RHS, II, Info)
+          .value_or(false)) {
+    II->replaceAllUsesWith(ConstantInt::get(II->getType(), 1));
+    ToRemove.push_back(II);
+    return true;
+  }
+  if (checkCondition(IsSigned ? ICmpInst::ICMP_SLT : ICmpInst::ICMP_ULT, LHS,
+                     RHS, II, Info)
+          .value_or(false)) {
+    II->replaceAllUsesWith(ConstantInt::getSigned(II->getType(), -1));
+    ToRemove.push_back(II);
+    return true;
+  }
+  if (checkCondition(ICmpInst::ICMP_EQ, LHS, RHS, II, Info).value_or(false)) {
+    II->replaceAllUsesWith(ConstantInt::get(II->getType(), 0));
+    ToRemove.push_back(II);
+    return true;
+  }
+  return false;
+}
+
 static void
 removeEntryFromStack(const StackEntry &E, ConstraintInfo &Info,
                      Module *ReproducerModule,
@@ -1736,6 +1765,11 @@ static bool eliminateConstraints(Function &F, DominatorTree &DT, LoopInfo &LI,
         Changed |= Simplified;
       } else if (auto *MinMax = dyn_cast<MinMaxIntrinsic>(Inst)) {
         Changed |= checkAndReplaceMinMax(MinMax, Info, ToRemove);
+      } else if (auto *CmpIntrinsic = dyn_cast<IntrinsicInst>(Inst);
+                 CmpIntrinsic &&
+                 (CmpIntrinsic->getIntrinsicID() == Intrinsic::scmp ||
+                  CmpIntrinsic->getIntrinsicID() == Intrinsic::ucmp)) {
+        Changed |= checkAndReplaceCmp(CmpIntrinsic, Info, ToRemove);
       }
       continue;
     }
diff --git a/llvm/test/Transforms/ConstraintElimination/uscmp.ll b/llvm/test/Transforms/ConstraintElimination/uscmp.ll
new file mode 100644
index 0000000000000..16ca93f0427e7
--- /dev/null
+++ b/llvm/test/Transforms/ConstraintElimination/uscmp.ll
@@ -0,0 +1,110 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -passes=constraint-elimination -S %s | FileCheck %s
+
+define i8 @scmp_1(i32 %x, i32 %y) {
+; CHECK-LABEL: @scmp_1(
+; CHECK-NEXT:    [[COND:%.*]] = icmp sgt i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    br i1 [[COND]], label [[TRUE:%.*]], label [[FALSE:%.*]]
+; CHECK:       true:
+; CHECK-NEXT:    ret i8 1
+; CHECK:       false:
+; CHECK-NEXT:    ret i8 20
+;
+  %cond = icmp sgt i32 %x, %y
+  br i1 %cond, label %true, label %false
+true:
+  %r = call i8 @llvm.scmp(i32 %x, i32 %y)
+  ret i8 %r
+false:
+  ret i8 20
+}
+
+define i8 @ucmp_1(i32 %x, i32 %y) {
+; CHECK-LABEL: @ucmp_1(
+; CHECK-NEXT:    [[COND:%.*]] = icmp ult i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    br i1 [[COND]], label [[TRUE:%.*]], label [[FALSE:%.*]]
+; CHECK:       true:
+; CHECK-NEXT:    ret i8 -1
+; CHECK:       false:
+; CHECK-NEXT:    ret i8 20
+;
+  %cond = icmp ult i32 %x, %y
+  br i1 %cond, label %true, label %false
+true:
+  %r = call i8 @llvm.ucmp(i32 %x, i32 %y)
+  ret i8 %r
+false:
+  ret i8 20
+}
+
+define i8 @scmp_2(i32 %x, i32 %y) {
+; CHECK-LABEL: @scmp_2(
+; CHECK-NEXT:    [[COND:%.*]] = icmp sge i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    br i1 [[COND]], label [[TRUE:%.*]], label [[FALSE:%.*]]
+; CHECK:       true:
+; CHECK-NEXT:    ret i8 20
+; CHECK:       false:
+; CHECK-NEXT:    ret i8 -1
+;
+  %cond = icmp sge i32 %x, %y
+  br i1 %cond, label %true, label %false
+true:
+  ret i8 20
+false:
+  %r = call i8 @llvm.scmp(i32 %x, i32 %y)
+  ret i8 %r
+}
+
+define i8 @ucmp_2(i32 %x, i32 %y) {
+; CHECK-LABEL: @ucmp_2(
+; CHECK-NEXT:    [[COND:%.*]] = icmp ule i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    br i1 [[COND]], label [[TRUE:%.*]], label [[FALSE:%.*]]
+; CHECK:       true:
+; CHECK-NEXT:    ret i8 20
+; CHECK:       false:
+; CHECK-NEXT:    ret i8 1
+;
+  %cond = icmp ule i32 %x, %y
+  br i1 %cond, label %true, label %false
+true:
+  ret i8 20
+false:
+  %r = call i8 @llvm.ucmp(i32 %x, i32 %y)
+  ret i8 %r
+}
+
+define i8 @scmp_3(i32 %x, i32 %y) {
+; CHECK-LABEL: @scmp_3(
+; CHECK-NEXT:    [[COND:%.*]] = icmp eq i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    br i1 [[COND]], label [[TRUE:%.*]], label [[FALSE:%.*]]
+; CHECK:       true:
+; CHECK-NEXT:    ret i8 0
+; CHECK:       false:
+; CHECK-NEXT:    ret i8 20
+;
+  %cond = icmp eq i32 %x, %y
+  br i1 %cond, label %true, label %false
+true:
+  %r = call i8 @llvm.scmp(i32 %x, i32 %y)
+  ret i8 %r
+false:
+  ret i8 20
+}
+
+define i8 @ucmp_3(i32 %x, i32 %y) {
+; CHECK-LABEL: @ucmp_3(
+; CHECK-NEXT:    [[COND:%.*]] = icmp eq i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    br i1 [[COND]], label [[TRUE:%.*]], label [[FALSE:%.*]]
+; CHECK:       true:
+; CHECK-NEXT:    ret i8 0
+; CHECK:       false:
+; CHECK-NEXT:    ret i8 20
+;
+  %cond = icmp eq i32 %x, %y
+  br i1 %cond, label %true, label %false
+true:
+  %r = call i8 @llvm.ucmp(i32 %x, i32 %y)
+  ret i8 %r
+false:
+  ret i8 20
+}

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

dtcxzyw · 2024-07-08T02:51:54Z

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

+  if (checkCondition(IsSigned ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT, LHS,
+                     RHS, II, Info)
+          .value_or(false)) {
+    II->replaceAllUsesWith(ConstantInt::get(II->getType(), 1));
+    ToRemove.push_back(II);
+    return true;
+  }
+  if (checkCondition(IsSigned ? ICmpInst::ICMP_SLT : ICmpInst::ICMP_ULT, LHS,
+                     RHS, II, Info)
+          .value_or(false)) {
+    II->replaceAllUsesWith(ConstantInt::getSigned(II->getType(), -1));
+    ToRemove.push_back(II);
+    return true;
+  }
+  if (checkCondition(ICmpInst::ICMP_EQ, LHS, RHS, II, Info).value_or(false)) {


We can avoid this check since !(x < y) && !(x > y) implies x == y.

Suggested change

if (checkCondition(IsSigned ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT, LHS,

RHS, II, Info)

.value_or(false)) {

II->replaceAllUsesWith(ConstantInt::get(II->getType(), 1));

ToRemove.push_back(II);

return true;

}

if (checkCondition(IsSigned ? ICmpInst::ICMP_SLT : ICmpInst::ICMP_ULT, LHS,

RHS, II, Info)

.value_or(false)) {

II->replaceAllUsesWith(ConstantInt::getSigned(II->getType(), -1));

ToRemove.push_back(II);

return true;

}

if (checkCondition(ICmpInst::ICMP_EQ, LHS, RHS, II, Info).value_or(false)) {

auto CmpGt = checkCondition(IsSigned ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT, LHS,

RHS, II, Info);

if (CmpGt.value_or(false)) {

II->replaceAllUsesWith(ConstantInt::get(II->getType(), 1));

ToRemove.push_back(II);

return true;

}

auto CmpLt = checkCondition(IsSigned ? ICmpInst::ICMP_SLT : ICmpInst::ICMP_ULT, LHS,

RHS, II, Info);

if (CmpLt.value_or(false)) {

II->replaceAllUsesWith(ConstantInt::getSigned(II->getType(), -1));

ToRemove.push_back(II);

return true;

}

if (CmpLt == false && CmpGt == false) {

It's true in principle, but implementing it this way wouldn't fold specifically scmp in situations where equality is implied.
E.g. this test case doesn't fold:

define i8 @test(i32 noundef %x, i32 noundef %y) { %cond = icmp eq i32 %x, %y call void @llvm.assume(i1 %cond) %r = call i8 @llvm.scmp.i8.i32(i32 %x, i32 %y) ret i8 %r }

I'm not sure why this is the case, but it looks like when constraint elimination state is fed an assumption that x == y it adds the two implied inequalities to its unsigned table, but not to the signed one. That's also why the same test above will fold nicely if you replace scmp with ucmp. Unless I'm missing something and this implication isn't necessarily correct I'm happy to change my code as per your suggestion and fix the contraint elimination pass in as a whole in a separate PR.

@fhahn Do you have a plan to support eq/ne with signed predicates?

define i1 @test1(i32 noundef %x, i32 noundef %y) { %cond = icmp eq i32 %x, %y call void @llvm.assume(i1 %cond) %r = icmp slt i32 %x, %y ret i1 %r } define i1 @test2(i32 noundef %x, i32 noundef %y) { %cond1 = icmp sle i32 %x, %y call void @llvm.assume(i1 %cond1) %cond2 = icmp sge i32 %x, %y call void @llvm.assume(i1 %cond2) %r = icmp eq i32 %x, %y ret i1 %r }

Unfortunately I don't have much spare cycles at the moment, but it should be a straight-forward extension. There are a number of cases where signed reasoning could be improved further :)

I think that, for the first example, it could suffice to check whether the equality holds and change the predicate to unsigned. Compile-time does not look good though: https://llvm-compile-time-tracker.com/compare.php?from=7911fb1a257b3a7014b44b4e7d04ee5c3b73a3e3&to=612eaf91c85d5e4c6e3fdddd46ebe6a41d19dfcb&stat=instructions:u. Perhaps there's a better way to achieve this?

For the second test case the simplest solution would probably be checking EQ/NE predicates against both constraint system. I'm not sure about the performace impact though and don't reallly know how to check it.

I think that, for the first example, it could suffice to check whether the equality holds and change the predicate to unsigned. Compile-time does not look good though: https://llvm-compile-time-tracker.com/compare.php?from=7911fb1a257b3a7014b44b4e7d04ee5c3b73a3e3&to=612eaf91c85d5e4c6e3fdddd46ebe6a41d19dfcb&stat=instructions:u. Perhaps there's a better way to achieve this?

doesHold(CmpInst::ICMP_EQ, Op0, Op1) will build another constraint system to solve. It's no surprise that the optimization slows down when you do this.
It would be better to add eq constraint into the signed system in ConstraintInfo::addFact.

Sorry about the misleading emoji :)

It would be better to add eq constraint into the signed system in ConstraintInfo::addFact.

Oh, sure! ~~Maybe adding sge and sle facts if eq predicate in transferToOtherSystem could work too.~~ Scratch this, it would solve the first example, but not the second one.

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

llvm/test/Transforms/ConstraintElimination/uscmp.ll

Poseydon42 · 2024-07-10T17:22:42Z

Rebased to use the now-merged #98177. I've left the equality check for now since I'm unsure whether the fixes suggested by @antoniofrighetto and me are viable. If they are then I'll file a separate PR with them and use the simpler equality check suggested by @dtcxzyw instead.

nikic

LGTM

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

fhahn

LGTM, thanks!

) This adds checks to fold calls to `ucmp`/`scmp` where a comparative relationship between the arguments can be established.

llvmbot added the llvm:transforms label Jul 7, 2024

dtcxzyw requested review from dtcxzyw, nikic and fhahn July 8, 2024 02:36

dtcxzyw reviewed Jul 8, 2024

View reviewed changes

fhahn reviewed Jul 8, 2024

View reviewed changes

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp Outdated Show resolved Hide resolved

llvm/test/Transforms/ConstraintElimination/uscmp.ll Show resolved Hide resolved

llvmbot added the llvm:ir label Jul 9, 2024

Poseydon42 added 2 commits July 10, 2024 15:48

[ConstraintElimination] Add support for UCMP/SCMP intrinsics

4cc8e51

Added negative tests for mismatched signedness

114c4f8

Poseydon42 force-pushed the uscmp-constraint-elimination branch from 6af26c0 to 114c4f8 Compare July 10, 2024 17:19

nikic approved these changes Jul 10, 2024

View reviewed changes

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp Outdated Show resolved Hide resolved

Variable renaming

48a31bf

fhahn approved these changes Jul 10, 2024

View reviewed changes

nikic merged commit d18ca43 into llvm:main Jul 10, 2024
5 of 6 checks passed

dtcxzyw mentioned this pull request Aug 23, 2024

[ConstraintElim] Fix miscompilation caused by PR97974 #105790

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ConstraintElimination] Add support for UCMP/SCMP intrinsics #97974

[ConstraintElimination] Add support for UCMP/SCMP intrinsics #97974

Poseydon42 commented Jul 7, 2024

llvmbot commented Jul 7, 2024 •

edited

Loading

dtcxzyw Jul 8, 2024

Poseydon42 Jul 8, 2024

dtcxzyw Jul 9, 2024

fhahn Jul 9, 2024

antoniofrighetto Jul 10, 2024

Poseydon42 Jul 10, 2024

dtcxzyw Jul 11, 2024

antoniofrighetto Jul 11, 2024 •

edited

Loading

Poseydon42 commented Jul 10, 2024

nikic left a comment

fhahn left a comment

[ConstraintElimination] Add support for UCMP/SCMP intrinsics #97974

[ConstraintElimination] Add support for UCMP/SCMP intrinsics #97974

Conversation

Poseydon42 commented Jul 7, 2024

llvmbot commented Jul 7, 2024 • edited Loading

dtcxzyw Jul 8, 2024

Choose a reason for hiding this comment

Poseydon42 Jul 8, 2024

Choose a reason for hiding this comment

dtcxzyw Jul 9, 2024

Choose a reason for hiding this comment

fhahn Jul 9, 2024

Choose a reason for hiding this comment

antoniofrighetto Jul 10, 2024

Choose a reason for hiding this comment

Poseydon42 Jul 10, 2024

Choose a reason for hiding this comment

dtcxzyw Jul 11, 2024

Choose a reason for hiding this comment

antoniofrighetto Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Poseydon42 commented Jul 10, 2024

nikic left a comment

Choose a reason for hiding this comment

fhahn left a comment

Choose a reason for hiding this comment

llvmbot commented Jul 7, 2024 •

edited

Loading

antoniofrighetto Jul 11, 2024 •

edited

Loading