[InstCombine] Set `samesign` when converting signed predicates into unsigned #112642

dtcxzyw · 2024-10-17T01:04:19Z

Alive2: https://alive2.llvm.org/ce/z/6cqdt-

…nsigned

llvmbot · 2024-10-17T01:04:50Z

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

Alive2: https://alive2.llvm.org/ce/z/6cqdt-

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

5 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp (+5-2)
(modified) llvm/test/Transforms/InstCombine/call-guard.ll (+1-1)
(modified) llvm/test/Transforms/InstCombine/icmp.ll (+1-1)
(modified) llvm/test/Transforms/InstCombine/indexed-gep-compares.ll (+1-1)
(modified) llvm/test/Transforms/InstCombine/phi-known-bits-operand-order.ll (+2-2)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 18a6fdcec1728e..a9c88444ba2571 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -6774,8 +6774,11 @@ Instruction *InstCombinerImpl::foldICmpUsingKnownBits(ICmpInst &I) {
   // have the same sign.
   if (I.isSigned() &&
       ((Op0Known.Zero.isNegative() && Op1Known.Zero.isNegative()) ||
-       (Op0Known.One.isNegative() && Op1Known.One.isNegative())))
-    return new ICmpInst(I.getUnsignedPredicate(), Op0, Op1);
+       (Op0Known.One.isNegative() && Op1Known.One.isNegative()))) {
+    ICmpInst *NewICmp = new ICmpInst(I.getUnsignedPredicate(), Op0, Op1);
+    NewICmp->setSameSign();
+    return NewICmp;
+  }
 
   return nullptr;
 }
diff --git a/llvm/test/Transforms/InstCombine/call-guard.ll b/llvm/test/Transforms/InstCombine/call-guard.ll
index 6b31c78118d0b8..bc5f319e64f7fe 100644
--- a/llvm/test/Transforms/InstCombine/call-guard.ll
+++ b/llvm/test/Transforms/InstCombine/call-guard.ll
@@ -43,7 +43,7 @@ define void @test_guard_adjacent_diff_cond2(i32 %V1, i32 %V2) {
 ; CHECK-NEXT:    [[TMP1:%.*]] = and i32 [[V1:%.*]], [[V2:%.*]]
 ; CHECK-NEXT:    [[TMP2:%.*]] = icmp slt i32 [[TMP1]], 0
 ; CHECK-NEXT:    [[AND:%.*]] = and i32 [[V1]], 255
-; CHECK-NEXT:    [[C:%.*]] = icmp ult i32 [[AND]], 129
+; CHECK-NEXT:    [[C:%.*]] = icmp samesign ult i32 [[AND]], 129
 ; CHECK-NEXT:    [[TMP3:%.*]] = and i1 [[TMP2]], [[C]]
 ; CHECK-NEXT:    call void (i1, ...) @llvm.experimental.guard(i1 [[TMP3]], i32 123) [ "deopt"() ]
 ; CHECK-NEXT:    ret void
diff --git a/llvm/test/Transforms/InstCombine/icmp.ll b/llvm/test/Transforms/InstCombine/icmp.ll
index 7cafb4885ff0ee..e44f7002438cb9 100644
--- a/llvm/test/Transforms/InstCombine/icmp.ll
+++ b/llvm/test/Transforms/InstCombine/icmp.ll
@@ -1457,7 +1457,7 @@ define <2 x i1> @test67vecinverse(<2 x i32> %x) {
 define i1 @test68(i32 %x) {
 ; CHECK-LABEL: @test68(
 ; CHECK-NEXT:    [[AND:%.*]] = and i32 [[X:%.*]], 127
-; CHECK-NEXT:    [[CMP:%.*]] = icmp ugt i32 [[AND]], 30
+; CHECK-NEXT:    [[CMP:%.*]] = icmp samesign ugt i32 [[AND]], 30
 ; CHECK-NEXT:    ret i1 [[CMP]]
 ;
   %and = and i32 %x, 127
diff --git a/llvm/test/Transforms/InstCombine/indexed-gep-compares.ll b/llvm/test/Transforms/InstCombine/indexed-gep-compares.ll
index 79511cf9e666ff..1110bbc5403e49 100644
--- a/llvm/test/Transforms/InstCombine/indexed-gep-compares.ll
+++ b/llvm/test/Transforms/InstCombine/indexed-gep-compares.ll
@@ -40,7 +40,7 @@ define ptr @test1_nuw(ptr %A, i32 %Offset) {
 ; CHECK:       bb:
 ; CHECK-NEXT:    [[RHS_IDX:%.*]] = phi i32 [ [[RHS_ADD:%.*]], [[BB]] ], [ [[TMP_IDX]], [[ENTRY:%.*]] ]
 ; CHECK-NEXT:    [[RHS_ADD]] = add nuw nsw i32 [[RHS_IDX]], 4
-; CHECK-NEXT:    [[COND:%.*]] = icmp ugt i32 [[RHS_IDX]], 400
+; CHECK-NEXT:    [[COND:%.*]] = icmp samesign ugt i32 [[RHS_IDX]], 400
 ; CHECK-NEXT:    br i1 [[COND]], label [[BB2:%.*]], label [[BB]]
 ; CHECK:       bb2:
 ; CHECK-NEXT:    [[RHS_PTR:%.*]] = getelementptr inbounds nuw i8, ptr [[A:%.*]], i32 [[RHS_IDX]]
diff --git a/llvm/test/Transforms/InstCombine/phi-known-bits-operand-order.ll b/llvm/test/Transforms/InstCombine/phi-known-bits-operand-order.ll
index 6a054688753500..9ff093635ad793 100644
--- a/llvm/test/Transforms/InstCombine/phi-known-bits-operand-order.ll
+++ b/llvm/test/Transforms/InstCombine/phi-known-bits-operand-order.ll
@@ -20,7 +20,7 @@ define void @phi_recurrence_start_first() {
 ; CHECK-NEXT:    br i1 [[COND_V2]], label [[FOR_COND11:%.*]], label [[FOR_COND26]]
 ; CHECK:       for.cond11:
 ; CHECK-NEXT:    [[I_1:%.*]] = phi i32 [ [[START]], [[IF_THEN]] ], [ [[STEP:%.*]], [[FOR_COND11]] ]
-; CHECK-NEXT:    [[CMP13:%.*]] = icmp ult i32 [[I_1]], 100
+; CHECK-NEXT:    [[CMP13:%.*]] = icmp samesign ult i32 [[I_1]], 100
 ; CHECK-NEXT:    [[STEP]] = add nuw nsw i32 [[I_1]], 1
 ; CHECK-NEXT:    br i1 [[CMP13]], label [[FOR_COND11]], label [[WHILE_END]]
 ; CHECK:       for.cond26:
@@ -68,7 +68,7 @@ define void @phi_recurrence_step_first() {
 ; CHECK-NEXT:    br i1 [[COND_V2]], label [[FOR_COND11:%.*]], label [[FOR_COND26]]
 ; CHECK:       for.cond11:
 ; CHECK-NEXT:    [[I_1:%.*]] = phi i32 [ [[STEP:%.*]], [[FOR_COND11]] ], [ [[START]], [[IF_THEN]] ]
-; CHECK-NEXT:    [[CMP13:%.*]] = icmp ult i32 [[I_1]], 100
+; CHECK-NEXT:    [[CMP13:%.*]] = icmp samesign ult i32 [[I_1]], 100
 ; CHECK-NEXT:    [[STEP]] = add nuw nsw i32 [[I_1]], 1
 ; CHECK-NEXT:    br i1 [[CMP13]], label [[FOR_COND11]], label [[WHILE_END]]
 ; CHECK:       for.cond26:

elhewaty

LGTM

nikic · 2024-10-17T07:01:48Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

@@ -6774,8 +6774,11 @@ Instruction *InstCombinerImpl::foldICmpUsingKnownBits(ICmpInst &I) {
  // have the same sign.
  if (I.isSigned() &&


Shouldn't we also set the flag if !isSigned()?

github-actions · 2024-10-17T08:24:25Z

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

nikic · 2024-10-17T10:11:35Z

llvm/test/Transforms/InstCombine/cast_phi.ll

@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+; RUN: opt < %s -passes="instcombine<no-verify-fixpoint>" -S | FileCheck %s


What's the issue here?

In function trunc_in_loop_exit_block:

define i8 @trunc_in_loop_exit_block() { ; CHECK-LABEL: @trunc_in_loop_exit_block( ; CHECK-NEXT: entry: ; CHECK-NEXT: br label [[LOOP:%.*]] ; CHECK: loop: ; CHECK-NEXT: [[IV:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[LOOP_LATCH:%.*]] ] ; CHECK-NEXT: [[PHI:%.*]] = phi i32 [ 1, [[ENTRY]] ], [ [[IV_NEXT]], [[LOOP_LATCH]] ] ; CHECK-NEXT: [[CMP:%.*]] = icmp samesign ult i32 [[IV]], 100 ; CHECK-NEXT: br i1 [[CMP]], label [[LOOP_LATCH]], label [[EXIT:%.*]] ; CHECK: loop.latch: ; CHECK-NEXT: [[IV_NEXT]] = add nuw nsw i32 [[IV]], 1 ; CHECK-NEXT: br label [[LOOP]] ; CHECK: exit: ; CHECK-NEXT: [[TRUNC:%.*]] = trunc i32 [[PHI]] to i8 ; CHECK-NEXT: ret i8 [[TRUNC]] ; entry: br label %loop loop: %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop.latch ] %phi = phi i32 [ 1, %entry ], [ %iv.next, %loop.latch ] %cmp = icmp ult i32 %iv, 100 br i1 %cmp, label %loop.latch, label %exit loop.latch: %iv.next = add i32 %iv, 1 br label %loop exit: %trunc = trunc i32 %phi to i8 ret i8 %trunc }

%iv u< 100 -> infer nsw/nuw for %iv.next = add i32 %iv, 1
-> %iv is non-negative -> infer samesign for %cmp = icmp ult i32 %iv, 100.

I see. This is a bit of recurring problem, may be worthwhile to add a special case at some point to revisit phi users if we add nuw to a recurrence add.

I'm also wondering if it would make sense to add an "instcombine-no-verify-fixpoint" function attribute, so that we can mark a single function instead of a whole file. This makes it easier to see which function is problematic and also avoids disabling verification for many unrelated tests...

nikic

LGTM

…#113822) This patch introduces a function attribute `instcombine-no-verify-fixpoint` to avoids disabling fix-point verification for unrelated tests in the same file. Address comment #112642 (comment).

…llvm#113822) This patch introduces a function attribute `instcombine-no-verify-fixpoint` to avoids disabling fix-point verification for unrelated tests in the same file. Address comment llvm#112642 (comment).

…erred (#113933) This patch re-queue users of phi when one of its incoming add instructions is updated. If an add instruction is updated, the analysis results of phis may be improved. Thus we may further fold some users of this phi node. See the following case: ``` define i8 @trunc_in_loop_exit_block() { ; CHECK-LABEL: @trunc_in_loop_exit_block( ; CHECK-NEXT: entry: ; CHECK-NEXT: br label [[LOOP:%.*]] ; CHECK: loop: ; CHECK-NEXT: [[IV:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[LOOP_LATCH:%.*]] ] ; CHECK-NEXT: [[PHI:%.*]] = phi i32 [ 1, [[ENTRY]] ], [ [[IV_NEXT]], [[LOOP_LATCH]] ] ; CHECK-NEXT: [[CMP:%.*]] = icmp samesign ult i32 [[IV]], 100 ; CHECK-NEXT: br i1 [[CMP]], label [[LOOP_LATCH]], label [[EXIT:%.*]] ; CHECK: loop.latch: ; CHECK-NEXT: [[IV_NEXT]] = add nuw nsw i32 [[IV]], 1 ; CHECK-NEXT: br label [[LOOP]] ; CHECK: exit: ; CHECK-NEXT: [[TRUNC:%.*]] = trunc i32 [[PHI]] to i8 ; CHECK-NEXT: ret i8 [[TRUNC]] ; entry: br label %loop loop: %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop.latch ] %phi = phi i32 [ 1, %entry ], [ %iv.next, %loop.latch ] %cmp = icmp ult i32 %iv, 100 br i1 %cmp, label %loop.latch, label %exit loop.latch: %iv.next = add i32 %iv, 1 br label %loop exit: %trunc = trunc i32 %phi to i8 ret i8 %trunc } ``` `%iv u< 100` -> infer `nsw/nuw` for `%iv.next = add i32 %iv, 1` -> `%iv` is non-negative -> infer `samesign` for `%cmp = icmp ult i32 %iv, 100`. Without re-queuing users of phi nodes, we cannot improve `%cmp` in one iteration. Address review comment #112642 (comment). This patch also fixes some non-fixpoint issues in tests.

[InstCombine] Set samesign when converting signed predicates into u…

5f45391

…nsigned

dtcxzyw requested review from antoniofrighetto, elhewaty and goldsteinn October 17, 2024 01:04

dtcxzyw requested a review from nikic as a code owner October 17, 2024 01:04

llvmbot added the llvm:transforms label Oct 17, 2024

dtcxzyw mentioned this pull request Oct 17, 2024

Task submission dtcxzyw/llvm-opt-benchmark#1312

Open

elhewaty approved these changes Oct 17, 2024

View reviewed changes

dtcxzyw mentioned this pull request Oct 17, 2024

pre-commit: PR112642 dtcxzyw/llvm-opt-benchmark#1508

Closed

nikic reviewed Oct 17, 2024

View reviewed changes

[InstCombine] Set samesign for unsigned predicates

961abf9

llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Oct 17, 2024

format

bd7ac7f

nikic reviewed Oct 17, 2024

View reviewed changes

nikic approved these changes Oct 17, 2024

View reviewed changes

dtcxzyw merged commit 095d49d into llvm:main Oct 17, 2024
8 checks passed

dtcxzyw deleted the perf/infer-samesign branch October 17, 2024 12:43

dtcxzyw mentioned this pull request Oct 27, 2024

[InstCombine] Add function attribute instcombine-no-verify-fixpoint #113822

Merged

dtcxzyw mentioned this pull request Oct 28, 2024

[InstCombine] Re-queue users of phi when nsw/nuw flags of add are inferred #113933

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[InstCombine] Set `samesign` when converting signed predicates into unsigned #112642

[InstCombine] Set `samesign` when converting signed predicates into unsigned #112642

Uh oh!

dtcxzyw commented Oct 17, 2024

Uh oh!

llvmbot commented Oct 17, 2024 •

edited

Loading

Uh oh!

elhewaty left a comment

Uh oh!

nikic Oct 17, 2024

Uh oh!

github-actions bot commented Oct 17, 2024 •

edited

Loading

Uh oh!

nikic Oct 17, 2024

Uh oh!

dtcxzyw Oct 17, 2024

Uh oh!

nikic Oct 17, 2024

Uh oh!

nikic Oct 17, 2024

Uh oh!

nikic left a comment

Uh oh!

Uh oh!

Uh oh!

		@@ -6774,8 +6774,11 @@ Instruction *InstCombinerImpl::foldICmpUsingKnownBits(ICmpInst &I) {
		// have the same sign.
		if (I.isSigned() &&

[InstCombine] Set samesign when converting signed predicates into unsigned #112642

[InstCombine] Set samesign when converting signed predicates into unsigned #112642

Uh oh!

Conversation

dtcxzyw commented Oct 17, 2024

Uh oh!

llvmbot commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elhewaty left a comment

Choose a reason for hiding this comment

Uh oh!

nikic Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

dtcxzyw Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

nikic Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

nikic Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[InstCombine] Set `samesign` when converting signed predicates into unsigned #112642

[InstCombine] Set `samesign` when converting signed predicates into unsigned #112642

llvmbot commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading