[InstCombine] Fold `icmp samesign u{gt/lt} (X +nsw C2), C` -> `icmp s{gt/lt} X, (C - C2)` #169960

wermos · 2025-11-28T20:50:36Z

Partially addresses #134028

Alive2 proof: https://alive2.llvm.org/ce/z/BqHQNN

github-actions · 2025-11-28T20:50:55Z

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.

llvmbot · 2025-11-28T20:51:28Z

@llvm/pr-subscribers-llvm-transforms

Author: Tirthankar Mazumder (wermos)

Changes

Fixes #166973

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp (+8)
(modified) llvm/test/Transforms/InstCombine/icmp-add.ll (+12)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 33eee8e059486..a707f1d2a056c 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -28,6 +28,7 @@
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
+#include "llvm/IR/IntrinsicsX86.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/Support/KnownBits.h"
 #include "llvm/Transforms/InstCombine/InstCombiner.h"
@@ -3189,6 +3190,13 @@ Instruction *InstCombinerImpl::foldICmpAddConstant(ICmpInst &Cmp,
     return new ICmpInst(ICmpInst::getSignedPredicate(Pred), X,
                         ConstantInt::get(Ty, C - *C2));
 
+  // Fold icmp samesign u{gt/ge/lt/le} (add nsw X, C2), C
+  //      -> icmp s{gt/ge/lt/le} X, (C - C2)
+  CmpPredicate CP(Pred, Cmp.hasSameSign());
+  if (CP.hasSameSign() && Add->hasNoSignedWrap())
+    return new ICmpInst(CP.getPreferredSignedPredicate(), X,
+                        ConstantInt::get(Ty, C - *C2));
+
   auto CR = ConstantRange::makeExactICmpRegion(Pred, C).subtract(*C2);
   const APInt &Upper = CR.getUpper();
   const APInt &Lower = CR.getLower();
diff --git a/llvm/test/Transforms/InstCombine/icmp-add.ll b/llvm/test/Transforms/InstCombine/icmp-add.ll
index 8449c7c5ea935..7c30337126b2b 100644
--- a/llvm/test/Transforms/InstCombine/icmp-add.ll
+++ b/llvm/test/Transforms/InstCombine/icmp-add.ll
@@ -3440,3 +3440,15 @@ define i1 @val_is_aligend_pred_mismatch(i32 %num) {
   %_0 = icmp sge i32 %num.masked, %num
   ret i1 %_0
 }
+
+define i1 @icmp_samesign_with_nsw_add(i32 %arg0) {
+; CHECK-LABEL: @icmp_samesign_with_nsw_add(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[V1:%.*]] = icmp sgt i32 [[ARG0:%.*]], 25
+; CHECK-NEXT:    ret i1 [[V1]]
+;
+entry:
+  %v0 = add nsw i32 %arg0, -18
+  %v1 = icmp samesign ugt i32 %v0, 7
+  ret i1 %v1
+}

wermos · 2025-11-28T20:59:39Z

I'm not sure if I need to add more tests or not.

Ping @dtcxzyw for enabling CI and code review

andjo403 · 2025-11-28T22:07:01Z

Good start is to look at https://llvm.org/docs/InstCombineContributorGuide.html
is missing a general proof like https://alive2.llvm.org/ce/z/dyczJy that shows that this code is missing handling of cases when the C - C2 calculation overflow.
there is a fold above the new fold that handled this case without the samesign flag.
also more test is needed eg. where there is signed and unsigned overflow of C - C2 calculation, test where add is "nuw nsw"

there also is this old #134556 PR that is trying to solve the same thing that have comments that is not handled.

wermos · 2025-11-29T07:58:14Z

Thanks for the pointers, I'll work on addressing those.

wermos · 2025-11-29T08:32:40Z

there is a fold above the new fold that handled this case without the samesign flag.

Are you suggesting that I should modify that code to handle the samesign case as well? Since that code uses computeConstantRange which I am not too familiar with, I opted to add a new if clause.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

wermos · 2025-11-30T11:32:13Z

I ran into an interesting problem wherein my implemented transform is interfering with the transform added in #95649. That transform deals with the specific case where C == -C2 (and C2 is a power of 2) and it opts to convert icmp ult (add X, C2), C into icmp ne (and X, C), 2C.

If I implement the transform where @andjo403 suggested, then tests like this:

llvm-project/llvm/test/Transforms/InstCombine/icmp-add.ll

Lines 3048 to 3057 in b228256

    
           define i1 @ult_add_nsw_C2_pow2_C_neg(i8 %x) { 
        
           ; CHECK-LABEL: @ult_add_nsw_C2_pow2_C_neg( 
        
           ; CHECK-NEXT:    [[TMP1:%.*]] = and i8 [[X:%.*]], -32 
        
           ; CHECK-NEXT:    [[C:%.*]] = icmp ne i8 [[TMP1]], -64 
        
           ; CHECK-NEXT:    ret i1 [[C]] 
        
           ; 
        
             %i = add nsw i8 %x, 32 
        
             %c = icmp ult i8 %i, -32 
        
             ret i1 %c 
        
           }

will get optimized differently.

This happens because the add folding code I am adding comes before the power-of-2 folding code.

So my question is, do we want to keep that power-of-2 behavior? If that is the case, then I will need to add extra conditions to the if-clause to prevent it from entering when C == -C2 and C2 is a power of 2.

wermos · 2025-11-30T11:33:15Z

I apologize for not having some code commits to go along with my previous comment, but I am working out some other unrelated kinks in my code, and I didn't want to unnecessarily confuse people.

andjo403 · 2025-11-30T12:02:25Z

sounds like a fault in your transform as the example that you give do not have samesign on the icmp so I do not think that it shall be affected by your transform

andjo403 · 2025-11-30T12:22:58Z

Maybe is not so easy to combine the transforms in to one transform but add the new transform after the other and copy the parts that is needed for when samesign is used

wermos · 2025-11-30T18:17:19Z

I've edited my topmost comment so it has an Alive2 proof link now.

wermos · 2025-11-30T18:21:15Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

    // TODO: Can we assert there is no overflow because InstSimplify always
    // handles those cases?


Can this comment be removed? If I understood this comment correctly, then it is obvious that there are cases where an overflow can happen, so this defensive code is needed. Specifically, the @icmp_samesign_with_nsw_add_neg test causes a ssub overflow to happen.

github-actions · 2025-11-30T18:50:14Z

🐧 Linux x64 Test Results

166573 tests passed
2878 tests skipped
1 test failed

Failed Tests

(click on a test name to see its output)

LLVM

LLVM.Transforms/InstCombine/remove-loop-phi-multiply-by-zero.ll

Exit Code: 1

Command Output (stdout):
--
# RUN: at line 2
/home/gha/actions-runner/_work/llvm-project/llvm-project/build/bin/opt < /home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/test/Transforms/InstCombine/remove-loop-phi-multiply-by-zero.ll -passes=instcombine -S | /home/gha/actions-runner/_work/llvm-project/llvm-project/build/bin/FileCheck /home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/test/Transforms/InstCombine/remove-loop-phi-multiply-by-zero.ll
# executed command: /home/gha/actions-runner/_work/llvm-project/llvm-project/build/bin/opt -passes=instcombine -S
# note: command had no output on stdout or stderr
# executed command: /home/gha/actions-runner/_work/llvm-project/llvm-project/build/bin/FileCheck /home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/test/Transforms/InstCombine/remove-loop-phi-multiply-by-zero.ll
# .---command stderr------------
# | /home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/test/Transforms/InstCombine/remove-loop-phi-multiply-by-zero.ll:212:15: error: CHECK-NEXT: expected string not found in input
# | ; CHECK-NEXT: [[CMP:%.*]] = icmp ult i64 [[I_02]], 999
# |               ^
# | <stdin>:117:33: note: scanning from here
# |  %inc = add nuw nsw i64 %i.02, 1
# |                                 ^
# | <stdin>:117:33: note: with "I_02" equal to "%i.02"
# |  %inc = add nuw nsw i64 %i.02, 1
# |                                 ^
# | <stdin>:118:2: note: possible intended match here
# |  %cmp = icmp slt i64 %i.02, 999
# |  ^
# | /home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/test/Transforms/InstCombine/remove-loop-phi-multiply-by-zero.ll:251:15: error: CHECK-NEXT: expected string not found in input
# | ; CHECK-NEXT: [[CMP:%.*]] = icmp ult i64 [[I_02]], 999
# |               ^
# | <stdin>:138:33: note: scanning from here
# |  %inc = add nuw nsw i64 %i.02, 1
# |                                 ^
# | <stdin>:138:33: note: with "I_02" equal to "%i.02"
# |  %inc = add nuw nsw i64 %i.02, 1
# |                                 ^
# | <stdin>:139:2: note: possible intended match here
# |  %cmp = icmp slt i64 %i.02, 999
# |  ^
# | /home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/test/Transforms/InstCombine/remove-loop-phi-multiply-by-zero.ll:358:15: error: CHECK-NEXT: expected string not found in input
# | ; CHECK-NEXT: [[CMP:%.*]] = icmp ult i64 [[I_02]], 999
# |               ^
# | <stdin>:195:33: note: scanning from here
# |  %inc = add nuw nsw i64 %i.02, 1
# |                                 ^
# | <stdin>:195:33: note: with "I_02" equal to "%i.02"
# |  %inc = add nuw nsw i64 %i.02, 1
# |                                 ^
# | <stdin>:196:2: note: possible intended match here
# |  %cmp = icmp slt i64 %i.02, 999
# |  ^
# | /home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/test/Transforms/InstCombine/remove-loop-phi-multiply-by-zero.ll:397:15: error: CHECK-NEXT: expected string not found in input
# | ; CHECK-NEXT: [[CMP:%.*]] = icmp ult i64 [[I_02]], 999
# |               ^
# | <stdin>:216:33: note: scanning from here
# |  %inc = add nuw nsw i64 %i.02, 1
# |                                 ^
# | <stdin>:216:33: note: with "I_02" equal to "%i.02"
# |  %inc = add nuw nsw i64 %i.02, 1
# |                                 ^
# | <stdin>:217:2: note: possible intended match here
# |  %cmp = icmp slt i64 %i.02, 999
# |  ^
# | 
# | Input file: <stdin>
# | Check file: /home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/test/Transforms/InstCombine/remove-loop-phi-multiply-by-zero.ll
# | 
# | -dump-input=help explains the following input dump.
# | 
# | Input was:
# | <<<<<<
# |             .
# |             .
# |             .
# |           112:  %i.02 = phi i64 [ 0, %entry ], [ 0, %entry_2 ], [ %inc, %for.body ] 
# |           113:  %f_prod.01 = phi double [ 0.000000e+00, %entry ], [ 0.000000e+00, %entry_2 ], [ %mul, %for.body ] 
# |           114:  %arrayidx = getelementptr inbounds nuw double, ptr %arr_d, i64 %i.02 
# |           115:  %0 = load double, ptr %arrayidx, align 8 
# |           116:  %mul = fmul fast double %f_prod.01, %0 
# |           117:  %inc = add nuw nsw i64 %i.02, 1 
# | next:212'0                                     X error: no match found
# | next:212'1                                       with "I_02" equal to "%i.02"
# |           118:  %cmp = icmp slt i64 %i.02, 999 
# | next:212'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | next:212'2      ?                               possible intended match
# |           119:  br i1 %cmp, label %for.body, label %end 
# | next:212'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |           120:  
# | next:212'0     ~
# |           121: end: ; preds = %for.body 
# | next:212'0     ~~~~~~~~~~~~~~~~~~~~~~~~~
# |           122:  ret double %mul 
# | next:212'0     ~~~~~~~~~~~~~~~~~
# |           123: } 
# | next:212'0     ~~
# |             .
# |             .
# |             .
# |           133:  %i.02 = phi i64 [ 0, %entry ], [ 0, %entry_2 ], [ %inc, %for.body ] 
# |           134:  %f_prod.01 = phi double [ 1.000000e+00, %entry ], [ 0.000000e+00, %entry_2 ], [ %mul, %for.body ] 
# |           135:  %arrayidx = getelementptr inbounds nuw double, ptr %arr_d, i64 %i.02 
# |           136:  %0 = load double, ptr %arrayidx, align 8 
# |           137:  %mul = fmul fast double %f_prod.01, %0 
# |           138:  %inc = add nuw nsw i64 %i.02, 1 
# | next:251'0                                     X error: no match found
# | next:251'1                                       with "I_02" equal to "%i.02"
# |           139:  %cmp = icmp slt i64 %i.02, 999 
# | next:251'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | next:251'2      ?                               possible intended match
# |           140:  br i1 %cmp, label %for.body, label %end 
# | next:251'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |           141:  
# | next:251'0     ~
# |           142: end: ; preds = %for.body 
# | next:251'0     ~~~~~~~~~~~~~~~~~~~~~~~~~
# |           143:  ret double %mul 
# | next:251'0     ~~~~~~~~~~~~~~~~~
# |           144: } 
# | next:251'0     ~~
# |             .
# |             .
# |             .
# |           190:  %i.02 = phi i64 [ 0, %entry ], [ 0, %entry_2 ], [ %inc, %for.body ] 
# |           191:  %f_prod.01 = phi i32 [ 0, %entry ], [ 0, %entry_2 ], [ %mul, %for.body ] 
# |           192:  %arrayidx = getelementptr inbounds nuw i32, ptr %arr_d, i64 %i.02 
# |           193:  %0 = load i32, ptr %arrayidx, align 4 
# |           194:  %mul = mul i32 %f_prod.01, %0 
# |           195:  %inc = add nuw nsw i64 %i.02, 1 
# | next:358'0                                     X error: no match found
# | next:358'1                                       with "I_02" equal to "%i.02"
# |           196:  %cmp = icmp slt i64 %i.02, 999 
# | next:358'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | next:358'2      ?                               possible intended match
# |           197:  br i1 %cmp, label %for.body, label %end 
# | next:358'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |           198:  
# | next:358'0     ~
# |           199: end: ; preds = %for.body 
# | next:358'0     ~~~~~~~~~~~~~~~~~~~~~~~~~
# |           200:  ret i32 %mul 
# | next:358'0     ~~~~~~~~~~~~~~
# |           201: } 
# | next:358'0     ~~
# |             .
# |             .
# |             .
# |           211:  %i.02 = phi i64 [ 0, %entry ], [ 0, %entry_2 ], [ %inc, %for.body ] 
# |           212:  %f_prod.01 = phi i32 [ 0, %entry ], [ 1, %entry_2 ], [ %mul, %for.body ] 
# |           213:  %arrayidx = getelementptr inbounds nuw i32, ptr %arr_d, i64 %i.02 
# |           214:  %0 = load i32, ptr %arrayidx, align 4 
# |           215:  %mul = mul i32 %f_prod.01, %0 
# |           216:  %inc = add nuw nsw i64 %i.02, 1 
# | next:397'0                                     X error: no match found
# | next:397'1                                       with "I_02" equal to "%i.02"
# |           217:  %cmp = icmp slt i64 %i.02, 999 
# | next:397'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | next:397'2      ?                               possible intended match
# |           218:  br i1 %cmp, label %for.body, label %end 
# | next:397'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |           219:  
# | next:397'0     ~
# |           220: end: ; preds = %for.body 
# | next:397'0     ~~~~~~~~~~~~~~~~~~~~~~~~~
# |           221:  ret i32 %mul 
# | next:397'0     ~~~~~~~~~~~~~~
# |           222: } 
# | next:397'0     ~~
# | >>>>>>
# `-----------------------------
# error: command failed with exit status: 1

--

If these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the infrastructure label.

wermos · 2025-11-30T19:05:18Z

Should I run update_test_check.py on this file also, or is this a genuine test failure?

andjo403 · 2025-11-30T19:43:09Z

you need to run update_test_check.py on that file also but it show a test that you are missing where the add is "add nuw nsw" so the same comment as in #134556 (comment)

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

wermos · 2025-12-01T08:32:36Z

so we should try both, with a preference for unsigned.

Is there a reason we give higher priority to the case where the add is unsigned?

andjo403 · 2025-12-01T16:50:00Z

unsigned is the more canonical form

… (C - C2)` whenever applicable.

wermos · 2025-12-01T18:49:07Z

I haven't handled the nuw + nsw case yet, I'll do that later because I am traveling right now.

wermos requested a review from nikic as a code owner November 28, 2025 20:50

llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Nov 28, 2025

wermos force-pushed the icmp-add branch from 7411424 to 46fdc59 Compare November 28, 2025 20:52

wermos changed the title ~~Fold icmp samesign u{gt/ge/lt/le} (X +nsw C2), C -> icmp s{gt/ge/lt/le} X, (C - C2)~~ [InstCombine] Fold icmp samesign u{gt/ge/lt/le} (X +nsw C2), C -> icmp s{gt/ge/lt/le} X, (C - C2) Nov 28, 2025

andjo403 reviewed Nov 29, 2025

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp Outdated Show resolved Hide resolved

wermos marked this pull request as draft November 29, 2025 19:33

wermos force-pushed the icmp-add branch 2 times, most recently from fe22d30 to f197e46 Compare November 30, 2025 11:44

Pre-commit tests

29ab236

wermos force-pushed the icmp-add branch from f197e46 to 726d4a9 Compare November 30, 2025 18:14

wermos marked this pull request as ready for review November 30, 2025 18:16

wermos commented Nov 30, 2025

View reviewed changes

wermos changed the title ~~[InstCombine] Fold icmp samesign u{gt/ge/lt/le} (X +nsw C2), C -> icmp s{gt/ge/lt/le} X, (C - C2)~~ [InstCombine] Fold icmp samesign u{gt/lt} (X +nsw C2), C -> icmp s{gt/lt} X, (C - C2) Nov 30, 2025

andjo403 reviewed Nov 30, 2025

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp Outdated Show resolved Hide resolved

Fold icmp samesign u{gt/lt} (add nsw X, C2), C -> `icmp s{gt/lt} X,…

445fdd5

… (C - C2)` whenever applicable.

wermos force-pushed the icmp-add branch from 726d4a9 to 445fdd5 Compare December 1, 2025 18:47

		// TODO: Can we assert there is no overflow because InstSimplify always
		// handles those cases?

[InstCombine] Fold icmp samesign u{gt/lt} (X +nsw C2), C -> icmp s{gt/lt} X, (C - C2) #169960

Are you sure you want to change the base?

[InstCombine] Fold icmp samesign u{gt/lt} (X +nsw C2), C -> icmp s{gt/lt} X, (C - C2) #169960

Conversation

wermos commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 28, 2025

Uh oh!

llvmbot commented Nov 28, 2025

Uh oh!

wermos commented Nov 28, 2025

Uh oh!

andjo403 commented Nov 28, 2025

Uh oh!

wermos commented Nov 29, 2025

Uh oh!

wermos commented Nov 29, 2025

Uh oh!

Uh oh!

wermos commented Nov 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wermos commented Nov 30, 2025

Uh oh!

andjo403 commented Nov 30, 2025

Uh oh!

andjo403 commented Nov 30, 2025

Uh oh!

wermos commented Nov 30, 2025

Uh oh!

wermos Nov 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🐧 Linux x64 Test Results

Failed Tests

LLVM

Uh oh!

wermos commented Nov 30, 2025

Uh oh!

andjo403 commented Nov 30, 2025

Uh oh!

Uh oh!

wermos commented Dec 1, 2025

Uh oh!

andjo403 commented Dec 1, 2025

Uh oh!

wermos commented Dec 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[InstCombine] Fold `icmp samesign u{gt/lt} (X +nsw C2), C` -> `icmp s{gt/lt} X, (C - C2)` #169960

[InstCombine] Fold `icmp samesign u{gt/lt} (X +nsw C2), C` -> `icmp s{gt/lt} X, (C - C2)` #169960

wermos commented Nov 28, 2025 •

edited

Loading

wermos commented Nov 30, 2025 •

edited

Loading

wermos Nov 30, 2025 •

edited

Loading

github-actions bot commented Nov 30, 2025 •

edited

Loading