[InstCombine] Fold `select Cond, not X, X` into `Cond ^ X` #90089

dtcxzyw · 2024-04-25T17:13:01Z

See the following example:

define i1 @src(i64 %x, i1 %y) {
  %1526 = icmp ne i64 %x, 0
  %1527 = icmp eq i64 %x, 0
  %sel = select i1 %y, i1 %1526, i1 %1527
  ret i1 %sel
}

define i1 @tgt(i64 %x, i1 %y) {
  %1527 = icmp eq i64 %x, 0
  %sel = xor i1 %y, %1527
  ret i1 %sel
}

I find that this pattern is common in C/C++/Rust code base.
This patch folds select Cond, Y, X into Cond ^ X iff:

X has the same type as Cond
X is poison -> Y is poison
X == !Y

Alive2: https://alive2.llvm.org/ce/z/hSmkHS

llvmbot · 2024-04-25T17:13:35Z

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

See the following example:

define i1 @<!-- -->src(i64 %x, i1 %y) {
  %1526 = icmp ne i64 %x, 0
  %1527 = icmp eq i64 %x, 0
  %sel = select i1 %y, i1 %1526, i1 %1527
  ret i1 %sel
}

define i1 @<!-- -->tgt(i64 %x, i1 %y) {
  %1527 = icmp eq i64 %x, 0
  %sel = xor i1 %y, %1527
  ret i1 %sel
}

I find that this pattern is common in C/C++/Rust code base.
This patch folds select Cond, Y, X into Cond ^ X iff:

X has the same type as Cond
X is poison -> Y is poison
X == !Y

Alive2: https://alive2.llvm.org/ce/z/hSmkHS

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp (+5)
(modified) llvm/test/Transforms/InstCombine/select-cmp.ll (+74)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 117eb7a1dcc933..69341c8d3f41cf 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -3985,5 +3985,10 @@ Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
     }
   }
 
+  if (CondVal->getType() == SI.getType() && impliesPoison(FalseVal, TrueVal) &&
+      isImpliedCondition(FalseVal, TrueVal, DL, /*LHSIsTrue=*/true) == false &&
+      isImpliedCondition(FalseVal, TrueVal, DL, /*LHSIsTrue=*/false) == true)
+    return BinaryOperator::CreateXor(CondVal, FalseVal);
+
   return nullptr;
 }
diff --git a/llvm/test/Transforms/InstCombine/select-cmp.ll b/llvm/test/Transforms/InstCombine/select-cmp.ll
index 711fac542179f8..3ec89af8d868c7 100644
--- a/llvm/test/Transforms/InstCombine/select-cmp.ll
+++ b/llvm/test/Transforms/InstCombine/select-cmp.ll
@@ -345,4 +345,78 @@ define i1 @icmp_no_common(i1 %c, i8 %x, i8 %y, i8 %z) {
   ret i1 %r
 }
 
+define i1 @test_select_inverse1(i64 %x, i1 %y) {
+; CHECK-LABEL: @test_select_inverse1(
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp eq i64 [[X:%.*]], 0
+; CHECK-NEXT:    [[SEL:%.*]] = xor i1 [[CMP2]], [[Y:%.*]]
+; CHECK-NEXT:    ret i1 [[SEL]]
+;
+  %cmp1 = icmp ne i64 %x, 0
+  %cmp2 = icmp eq i64 %x, 0
+  %sel = select i1 %y, i1 %cmp1, i1 %cmp2
+  ret i1 %sel
+}
+
+define i1 @test_select_inverse2(i64 %x, i1 %y) {
+; CHECK-LABEL: @test_select_inverse2(
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp slt i64 [[X:%.*]], 0
+; CHECK-NEXT:    [[SEL:%.*]] = xor i1 [[CMP2]], [[Y:%.*]]
+; CHECK-NEXT:    ret i1 [[SEL]]
+;
+  %cmp1 = icmp sgt i64 %x, -1
+  %cmp2 = icmp slt i64 %x, 0
+  %sel = select i1 %y, i1 %cmp1, i1 %cmp2
+  ret i1 %sel
+}
+
+define i1 @test_select_inverse3(ptr %x, i1 %y) {
+; CHECK-LABEL: @test_select_inverse3(
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp ne ptr [[X:%.*]], null
+; CHECK-NEXT:    [[SEL:%.*]] = xor i1 [[CMP2]], [[Y:%.*]]
+; CHECK-NEXT:    ret i1 [[SEL]]
+;
+  %cmp1 = icmp eq ptr %x, null
+  %cmp2 = icmp ne ptr %x, null
+  %sel = select i1 %y, i1 %cmp1, i1 %cmp2
+  ret i1 %sel
+}
+
+define i1 @test_select_inverse_fail(i64 %x, i1 %y) {
+; CHECK-LABEL: @test_select_inverse_fail(
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp sgt i64 [[X:%.*]], 0
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp slt i64 [[X]], 0
+; CHECK-NEXT:    [[SEL:%.*]] = select i1 [[Y:%.*]], i1 [[CMP1]], i1 [[CMP2]]
+; CHECK-NEXT:    ret i1 [[SEL]]
+;
+  %cmp1 = icmp sgt i64 %x, 0
+  %cmp2 = icmp slt i64 %x, 0
+  %sel = select i1 %y, i1 %cmp1, i1 %cmp2
+  ret i1 %sel
+}
+
+define <2 x i1> @test_select_inverse_vec(<2 x i64> %x, <2 x i1> %y) {
+; CHECK-LABEL: @test_select_inverse_vec(
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp eq <2 x i64> [[X:%.*]], zeroinitializer
+; CHECK-NEXT:    [[SEL:%.*]] = xor <2 x i1> [[CMP2]], [[Y:%.*]]
+; CHECK-NEXT:    ret <2 x i1> [[SEL]]
+;
+  %cmp1 = icmp ne <2 x i64> %x, zeroinitializer
+  %cmp2 = icmp eq <2 x i64> %x, zeroinitializer
+  %sel = select <2 x i1> %y, <2 x i1> %cmp1, <2 x i1> %cmp2
+  ret <2 x i1> %sel
+}
+
+define <2 x i1> @test_select_inverse_vec_fail(<2 x i64> %x, i1 %y) {
+; CHECK-LABEL: @test_select_inverse_vec_fail(
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp ne <2 x i64> [[X:%.*]], zeroinitializer
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp eq <2 x i64> [[X]], zeroinitializer
+; CHECK-NEXT:    [[SEL:%.*]] = select i1 [[Y:%.*]], <2 x i1> [[CMP1]], <2 x i1> [[CMP2]]
+; CHECK-NEXT:    ret <2 x i1> [[SEL]]
+;
+  %cmp1 = icmp ne <2 x i64> %x, zeroinitializer
+  %cmp2 = icmp eq <2 x i64> %x, zeroinitializer
+  %sel = select i1 %y, <2 x i1> %cmp1, <2 x i1> %cmp2
+  ret <2 x i1> %sel
+}
+
 declare void @use(i1)

PR Link: llvm/llvm-project#90089

nikic · 2024-04-26T02:33:58Z

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

@@ -3985,5 +3985,10 @@ Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
    }
  }

+  if (CondVal->getType() == SI.getType() && impliesPoison(FalseVal, TrueVal) &&
+      isImpliedCondition(FalseVal, TrueVal, DL, /*LHSIsTrue=*/true) == false &&
+      isImpliedCondition(FalseVal, TrueVal, DL, /*LHSIsTrue=*/false) == true)


Would it cover the motivating cases to check for icmp with same operands and inverted predicate?

This is a pretty weird check and I'm not sure if doing the poison check in one direction is sufficient here.

Hm, I guess it's fine as FalseVal is the value we're replacing with.

I'd still like an answer to this question. This is a very roundabout way to check whether the conditions are inverses of each other, and it does have some overhead (http://llvm-compile-time-tracker.com/compare.php?from=76a3be7c766bd55221c3d0d0a74c42f82c5d76ed&to=1731798e2ef457fb5cabd29ae054c700b8b13c14&stat=instructions%3Au). Not enough to make me overly concerned, about enough to contribute to the death by a thousand cuts :)

I can simplify the implementation to just handle select Cond, icmp X, C1, icmp X, C2, which covers all the motivating cases.

goldsteinn · 2024-05-02T17:53:52Z

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

@@ -3985,5 +3985,10 @@ Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
    }
  }

+  if (CondVal->getType() == SI.getType() && impliesPoison(FalseVal, TrueVal) &&
+      isImpliedCondition(FalseVal, TrueVal, DL, /*LHSIsTrue=*/true) == false &&
+      isImpliedCondition(FalseVal, TrueVal, DL, /*LHSIsTrue=*/false) == true)


Shouldnt the check be (isImpliedCondition(..., true) ^ isImpliedCondition(..., false))?

When FVal implies TVal and !FVal -> !TVal, TVal must be the same as FVal. If not, we need a canonicalization for this pattern.

BTW, I tried isImpliedCondition(FalseVal, TrueVal, DL, /*LHSIsTrue=*/true) == true && isImpliedCondition(FalseVal, TrueVal, DL, /*LHSIsTrue=*/false) == false before submitting this patch. But it didn't improve the result.

Maybe add a comment that the inverse case doesn't naturally occur in code to avoid people re-testing / re-submitting.

RSilicon · 2024-05-16T00:53:50Z

This food gives me a few more ideas. Particularly it takes me back to that rejected PR. Perhaps I could use this property somehow to fold expressions regarding bitwise nots of each other...

goldsteinn · 2024-05-17T15:28:35Z

LGTM, wait for 1 more

dtcxzyw · 2024-05-18T02:40:04Z

TEST 'BOLT :: RISCV/unnamed-sym-no-entry.c' FAILED

The test failure is unrelated with this patch.

goldsteinn · 2024-05-18T06:07:41Z

TEST 'BOLT :: RISCV/unnamed-sym-no-entry.c' FAILED

The test failure is unrelated with this patch.

I've also seen that test fail on a few buildbots.

dtcxzyw · 2024-05-27T18:07:11Z

Ping @nikic

antoniofrighetto · 2024-05-27T18:11:37Z

LGTM!

dtcxzyw · 2024-06-04T13:17:57Z

Closing in favor of #93591.

dtcxzyw added 2 commits April 26, 2024 01:04

[InstCombine] Add pre-commit tests. NFC.

6dffd8b

[InstCombine] Fold select Cond, not X, X into Cond ^ X

ac7ded4

dtcxzyw requested a review from goldsteinn April 25, 2024 17:13

dtcxzyw requested a review from nikic as a code owner April 25, 2024 17:13

llvmbot added the llvm:transforms label Apr 25, 2024

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Apr 25, 2024

pre-commit: test PR90089

381ed9b

PR Link: llvm/llvm-project#90089

dtcxzyw mentioned this pull request Apr 25, 2024

pre-commit: test PR90089 dtcxzyw/llvm-opt-benchmark#557

Closed

nikic reviewed Apr 26, 2024

View reviewed changes

goldsteinn reviewed May 2, 2024

View reviewed changes

[InstCombine] Add a header comment for the fold. NFC.

d1e3e81

[InstCombine] Add some comments.

1731798

dtcxzyw mentioned this pull request May 28, 2024

[InstCombine] Fold select Cond, not X, X into Cond ^ X #93591

Merged

dtcxzyw closed this Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InstCombine] Fold `select Cond, not X, X` into `Cond ^ X` #90089

[InstCombine] Fold `select Cond, not X, X` into `Cond ^ X` #90089

dtcxzyw commented Apr 25, 2024

llvmbot commented Apr 25, 2024

nikic Apr 26, 2024

nikic Apr 26, 2024

nikic May 27, 2024

dtcxzyw May 27, 2024

goldsteinn May 2, 2024

dtcxzyw May 13, 2024

goldsteinn May 13, 2024

RSilicon commented May 16, 2024

goldsteinn commented May 17, 2024

dtcxzyw commented May 18, 2024

goldsteinn commented May 18, 2024

dtcxzyw commented May 27, 2024

antoniofrighetto commented May 27, 2024

dtcxzyw commented Jun 4, 2024

[InstCombine] Fold select Cond, not X, X into Cond ^ X #90089

[InstCombine] Fold select Cond, not X, X into Cond ^ X #90089

Conversation

dtcxzyw commented Apr 25, 2024

llvmbot commented Apr 25, 2024

nikic Apr 26, 2024

Choose a reason for hiding this comment

nikic Apr 26, 2024

Choose a reason for hiding this comment

nikic May 27, 2024

Choose a reason for hiding this comment

dtcxzyw May 27, 2024

Choose a reason for hiding this comment

goldsteinn May 2, 2024

Choose a reason for hiding this comment

dtcxzyw May 13, 2024

Choose a reason for hiding this comment

goldsteinn May 13, 2024

Choose a reason for hiding this comment

RSilicon commented May 16, 2024

goldsteinn commented May 17, 2024

dtcxzyw commented May 18, 2024

goldsteinn commented May 18, 2024

dtcxzyw commented May 27, 2024

antoniofrighetto commented May 27, 2024

dtcxzyw commented Jun 4, 2024

[InstCombine] Fold `select Cond, not X, X` into `Cond ^ X` #90089

[InstCombine] Fold `select Cond, not X, X` into `Cond ^ X` #90089