[ConstraintElim] Teach checkAndReplaceCondition about samesign #128168

citymarina · 2025-02-21T12:22:51Z

samesign upred is equivalent to spred: https://alive2.llvm.org/ce/z/57iaEC

llvmbot · 2025-02-21T12:23:30Z

@llvm/pr-subscribers-llvm-transforms

Author: Marina Taylor (citymarina)

Changes

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/ConstraintElimination.cpp (+8-1)
(modified) llvm/test/Transforms/ConstraintElimination/transfer-samesign-facts.ll (+20)

diff --git a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
index 6dd26910f6846..9a1a3f3dfeabc 100644
--- a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
@@ -1427,7 +1427,7 @@ static std::optional<bool> checkCondition(CmpInst::Predicate Pred, Value *A,
 }
 
 static bool checkAndReplaceCondition(
-    CmpInst *Cmp, ConstraintInfo &Info, unsigned NumIn, unsigned NumOut,
+    ICmpInst *Cmp, ConstraintInfo &Info, unsigned NumIn, unsigned NumOut,
     Instruction *ContextInst, Module *ReproducerModule,
     ArrayRef<ReproducerEntry> ReproducerCondStack, DominatorTree &DT,
     SmallVectorImpl<Instruction *> &ToRemove) {
@@ -1460,6 +1460,13 @@ static bool checkAndReplaceCondition(
           checkCondition(Cmp->getPredicate(), Cmp->getOperand(0),
                          Cmp->getOperand(1), Cmp, Info))
     return ReplaceCmpWithConstant(Cmp, *ImpliedCondition);
+
+  if (Cmp->hasSameSign())
+    if (auto ImpliedCondition = checkCondition(
+            ICmpInst::getFlippedSignednessPredicate(Cmp->getPredicate()),
+            Cmp->getOperand(0), Cmp->getOperand(1), Cmp, Info))
+      return ReplaceCmpWithConstant(Cmp, *ImpliedCondition);
+
   return false;
 }
 
diff --git a/llvm/test/Transforms/ConstraintElimination/transfer-samesign-facts.ll b/llvm/test/Transforms/ConstraintElimination/transfer-samesign-facts.ll
index 1b155e050de29..09cbbf1c661c7 100644
--- a/llvm/test/Transforms/ConstraintElimination/transfer-samesign-facts.ll
+++ b/llvm/test/Transforms/ConstraintElimination/transfer-samesign-facts.ll
@@ -303,3 +303,23 @@ define i1 @ugt_assumed_positive_values(i8 %a, i8 %b) {
 
   ret i1 %result
 }
+
+define i1 @samesign_flipped_signedness(i8 %a)  {
+; CHECK-LABEL: @samesign_flipped_signedness(
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp ugt i8 [[A:%.*]], 0
+; CHECK-NEXT:    br i1 [[TMP1]], label [[GREATER:%.*]], label [[EXIT:%.*]]
+; CHECK:       greater:
+; CHECK-NEXT:    ret i1 true
+; CHECK:       exit:
+; CHECK-NEXT:    ret i1 false
+;
+  %1 = icmp ugt i8 %a, 0
+  br i1 %1, label %greater, label %exit
+
+greater:
+  %2 = icmp samesign sgt i8 %a, 0
+  ret i1 %2
+
+exit:
+  ret i1 false
+}

dtcxzyw · 2025-02-21T15:02:14Z

I implemented the same feature last year :) 33c92a0
Maybe it caused some regressions in real-world programs.

Compile-time impact: https://llvm-compile-time-tracker.com/compare.php?from=d1a7225076218ce224cd29c74259b715b393dc9d&to=33c92a07b3a831778592b60962cced91d0b07275&stat=instructions:u

dtcxzyw · 2025-02-21T15:42:04Z

Maybe it caused some regressions in real-world programs.

IR diff looks OK.

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

llvm/test/Transforms/ConstraintElimination/transfer-samesign-facts.ll

artagnon

Looks reasonable, but I think the test coverage is lacking: how about a dominating samesign condition as well?

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

llvm/test/Transforms/ConstraintElimination/transfer-samesign-facts.ll

citymarina · 2025-02-21T17:47:31Z

Looks reasonable, but I think the test coverage is lacking: how about a dominating samesign condition as well?

Does the added test from #120089 (comment) cover the scenario that you were looking for? If not, could you sketch out for me what you had in mind?

artagnon

Almost there.

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

llvm/test/Transforms/ConstraintElimination/transfer-samesign-facts.ll

artagnon · 2025-02-21T17:53:56Z

llvm/test/Transforms/ConstraintElimination/transfer-samesign-facts.ll

+  %cmp_ult = icmp samesign ult i32 %a, 65
+  br i1 %cmp_ult, label %for.cond.preheader, label %if.else
+
+for.cond.preheader:
+  ret i1 false
+
+if.else:
+  %cmp_ugt = icmp samesign ugt i32 %a, -65
+  ret i1 %cmp_ugt


Note: -65 is interpreted as INT_MAX - 64 unsigned, and this wouldn't have been simplified to false before samesign support. However, the samesign on ult is redundant. Typically, we want two tests: one of the icmps should have samesign, and the other shouldn't.

Putting together the recent comments:

Exactly one of the icmps should have samesign

samesign only goes with unsigned

Want a dominating samesign test

that lead me to write:

define i1 @implied_condition_ugt_sgt(i8 %a, i8 %b) { %cmp_ugt = icmp samesign ugt i8 %a, %b br i1 %cmp_ugt, label %greater, label %exit greater: %cmp_sgt = icmp sgt i8 %a, %b ret i1 %cmp_sgt exit: ret i1 false }

but in this code, greater: is already optimized to ret i1 true even without this patch. Have I misunderstood some part?

Sorry, my bad! There are already tests for this. If you could just write one test with %a and %b and another with %a and a constant, that would be good.

Does the latest update capture what you had in mind?

llvm/test/Transforms/ConstraintElimination/transfer-samesign-facts.ll

artagnon · 2025-02-21T19:00:10Z

Could you also re-title this PR "[ConstraintElim] Teach checkAndReplaceCondition about samesign", as there are already other patches that teach other parts of ConstraintElim about samesign, and we want to disambiguate.

artagnon

LGTM, thanks!

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

fhahn · 2025-02-24T10:08:47Z

llvm/test/Transforms/ConstraintElimination/transfer-samesign-facts.ll

+  br i1 %cmp_sle, label %less_or_equal, label %exit
+
+less_or_equal:
+  %cmp_ule = icmp samesign ule i8 %a, 42


could you also add a test with samesign that cannot be simplified to also guard the negative case?

And maybe a variant with samesign on the signed known condition to also cover the case where we have samesign but a single predicate?

And maybe a variant with samesign on the signed known condition

Based on comments above, I thought samesign mustn't go on signed predicates?

Unless I misunderstood something, @fhahn: samesign is always canonicalized with unsigned predicates, and we generally avoid writing tests with samesign and signed predicates.

Yep, AFAIK you can have samesign with signed predicates in general, it looks like ConstraintElimination already handles those case (https://llvm.godbolt.org/z/fhs3r5M56), so this may already be covered by tests

fhahn

LGTM, thanks!

When landing, could you add a brief description to the commit message, perhaps including a link to an alive2 proof for the motivating example?

citymarina · 2025-02-24T12:30:59Z

Thanks, I've updated the description. If this is good to go, I could use some help to land as I don't yet have write access (I'll probably have enough commits to request it after this).

citymarina · 2025-02-24T14:09:10Z

Thank you!

llvmbot added the llvm:transforms label Feb 21, 2025

citymarina mentioned this pull request Feb 21, 2025

[ConstraintElim] NE implies SLT if SLE and SGT if SGE #127663

Closed

citymarina force-pushed the ce-ne-zero-3 branch from 95b8c00 to b4a5228 Compare February 21, 2025 13:47

dtcxzyw requested review from nikic and fhahn February 21, 2025 14:56

This was referenced Feb 21, 2025

Task submission dtcxzyw/llvm-opt-benchmark#1312

Open

pre-commit: PR128168 dtcxzyw/llvm-opt-benchmark#2151

Closed

dtcxzyw linked an issue Feb 21, 2025 that may be closed by this pull request

[ConstraintElim] icmp spred x, y implies icmp samesign upred x, y #120089

Closed

dtcxzyw requested review from artagnon and dtcxzyw February 21, 2025 15:35

dtcxzyw reviewed Feb 21, 2025

View reviewed changes

artagnon reviewed Feb 21, 2025

View reviewed changes

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

llvm/test/Transforms/ConstraintElimination/transfer-samesign-facts.ll Outdated Show resolved Hide resolved

citymarina force-pushed the ce-ne-zero-3 branch from b4a5228 to 1264d23 Compare February 21, 2025 17:45

artagnon reviewed Feb 21, 2025

View reviewed changes

citymarina changed the title ~~[ConstraintElim] Check the other signedness when hasSameSign~~ [ConstraintElim] Teach checkAndReplaceCondition about samesign Feb 21, 2025

citymarina force-pushed the ce-ne-zero-3 branch from 1264d23 to 8f3d2de Compare February 23, 2025 21:26

artagnon approved these changes Feb 24, 2025

View reviewed changes

fhahn reviewed Feb 24, 2025

View reviewed changes

citymarina added 2 commits February 24, 2025 11:59

[ConstraintElim] Add samesign test. NFC

fb802ed

[ConstraintElim] Teach checkAndReplaceCondition about samesign

dbe26da

citymarina force-pushed the ce-ne-zero-3 branch from 8f3d2de to dbe26da Compare February 24, 2025 12:00

fhahn approved these changes Feb 24, 2025

View reviewed changes

fhahn merged commit 72768d9 into llvm:main Feb 24, 2025
11 checks passed

citymarina deleted the ce-ne-zero-3 branch May 12, 2025 16:07

[ConstraintElim] Teach checkAndReplaceCondition about samesign #128168

[ConstraintElim] Teach checkAndReplaceCondition about samesign #128168

Uh oh!

Conversation

citymarina commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 21, 2025

Uh oh!

dtcxzyw commented Feb 21, 2025

Uh oh!

dtcxzyw commented Feb 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artagnon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

citymarina commented Feb 21, 2025

Uh oh!

artagnon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artagnon commented Feb 21, 2025

Uh oh!

artagnon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

citymarina commented Feb 24, 2025

Uh oh!

Uh oh!

citymarina commented Feb 24, 2025

Uh oh!

Uh oh!

citymarina commented Feb 21, 2025 •

edited

Loading