Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[InstCombine] Fold (X==Z) ? (Y==Z) : (!(Y==Z) && X==Y) --> X==Y #108619

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

citymarina
Copy link
Contributor

@citymarina citymarina commented Sep 13, 2024

This corresponds to the canonicalized form of some logic that was
seen in Swift-generated code for comparing optional pointers:
(X==Z || Y==Z) ? (X==Z && Y==Z) : X==Y --> X==Y
where Z was the constant 0.

https://alive2.llvm.org/ce/z/J_3aa9

@llvmbot
Copy link
Member

llvmbot commented Sep 13, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Marina Taylor (citymarina)

Changes

This logic was seen in code generated by Swift for comparing optional pointers,
with Z being the constant 0.

https://alive2.llvm.org/ce/z/Bpd8Yo


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

3 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineInternal.h (+1)
  • (modified) llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp (+45)
  • (added) llvm/test/Transforms/InstCombine/icmp-equality-test.ll (+202)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
index a051a568bfd62e..9bd06a0abb5e53 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
+++ b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
@@ -734,6 +734,7 @@ class LLVM_LIBRARY_VISIBILITY InstCombinerImpl final
   Instruction *foldSelectOfBools(SelectInst &SI);
   Instruction *foldSelectToCmp(SelectInst &SI);
   Instruction *foldSelectExtConst(SelectInst &Sel);
+  Instruction *foldSelectEqualityTest(SelectInst &SI);
   Instruction *foldSelectOpOp(SelectInst &SI, Instruction *TI, Instruction *FI);
   Instruction *foldSelectIntoOp(SelectInst &SI, Value *, Value *);
   Instruction *foldSPFofSPF(Instruction *Inner, SelectPatternFlavor SPF1,
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 66f7c4592457c2..0020054c775e8e 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -1406,6 +1406,46 @@ Instruction *InstCombinerImpl::foldSelectValueEquivalence(SelectInst &Sel,
   return nullptr;
 }
 
+/// Fold the following code sequence:
+/// \code
+///   %XEq = icmp eq i64 %X, %Z
+///   %YEq = icmp eq i64 %Y, %Z
+///   %either = select i1 %XEq, i1 true, i1 %YEq
+///   %both = select i1 %XEq, i1 %YEq, i1 false
+///   %cmp = icmp eq i64 %X, %Y
+///   %equal = select i1 %either, i1 %both, i1 %cmp
+/// \code
+///
+/// into:
+///   %equal = icmp eq i64 %X, %Y
+///
+/// Equivalently:
+///   (X==Z || Y==Z) ? (X==Z && Y==Z) : X==Y --> X==Y
+Instruction *InstCombinerImpl::foldSelectEqualityTest(SelectInst &Sel) {
+  Value *X, *Y, *Z, *XEq, *YEq;
+  Value *Either = Sel.getCondition(), *Both = Sel.getTrueValue(),
+        *Cmp = Sel.getFalseValue();
+
+  if (!match(Either, m_Select(m_Value(XEq), m_One(), m_Value(YEq))))
+    return nullptr;
+
+  if (!match(XEq,
+             m_c_SpecificICmp(ICmpInst::ICMP_EQ, m_Value(X), m_Value(Z))) ||
+      !match(YEq,
+             m_c_SpecificICmp(ICmpInst::ICMP_EQ, m_Value(Y), m_Specific(Z))))
+    return nullptr;
+
+  if (!match(Both, m_Select(m_Specific(XEq), m_Specific(YEq), m_Zero())) &&
+      !match(Both, m_Select(m_Specific(YEq), m_Specific(XEq), m_Zero())))
+    return nullptr;
+
+  if (!match(Cmp,
+             m_c_SpecificICmp(ICmpInst::ICMP_EQ, m_Specific(X), m_Specific(Y))))
+    return nullptr;
+
+  return replaceInstUsesWith(Sel, Cmp);
+}
+
 // See if this is a pattern like:
 //   %old_cmp1 = icmp slt i32 %x, C2
 //   %old_replacement = select i1 %old_cmp1, i32 %target_low, i32 %target_high
@@ -4068,6 +4108,11 @@ Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
   if (Instruction *I = foldSelectOfSymmetricSelect(SI, Builder))
     return I;
 
+  // This needs to happen before foldNestedSelects, as that could break the
+  // patterns that we test for.
+  if (Instruction *I = foldSelectEqualityTest(SI))
+    return I;
+
   if (Instruction *I = foldNestedSelects(SI, Builder))
     return I;
 
diff --git a/llvm/test/Transforms/InstCombine/icmp-equality-test.ll b/llvm/test/Transforms/InstCombine/icmp-equality-test.ll
new file mode 100644
index 00000000000000..1ee2db4cb318b6
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/icmp-equality-test.ll
@@ -0,0 +1,202 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
+define i1 @icmp_equality_test(i64 %X, i64 %Y, i64 %Z) {
+; CHECK-LABEL: @icmp_equality_test(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i64 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    ret i1 [[CMP]]
+;
+entry:
+  %XEq = icmp eq i64 %X, %Z
+  %YEq = icmp eq i64 %Y, %Z
+  %either = select i1 %XEq, i1 true, i1 %YEq
+  %both = select i1 %XEq, i1 %YEq, i1 false
+  %cmp = icmp eq i64 %X, %Y
+  %equal = select i1 %either, i1 %both, i1 %cmp
+  ret i1 %equal
+}
+
+define i1 @icmp_equality_test_constant(i42 %X, i42 %Y) {
+; CHECK-LABEL: @icmp_equality_test_constant(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i42 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    ret i1 [[CMP]]
+;
+entry:
+  %XEq = icmp eq i42 %X, -42
+  %YEq = icmp eq i42 %Y, -42
+  %either = select i1 %XEq, i1 true, i1 %YEq
+  %both = select i1 %XEq, i1 %YEq, i1 false
+  %cmp = icmp eq i42 %X, %Y
+  %equal = select i1 %either, i1 %both, i1 %cmp
+  ret i1 %equal
+}
+
+define <2 x i1> @icmp_equality_test_vector(<2 x i64> %X, <2 x i64> %Y) {
+; CHECK-LABEL: @icmp_equality_test_vector(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq <2 x i64> [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    ret <2 x i1> [[CMP]]
+;
+entry:
+  %XEq = icmp eq <2 x i64> %X, <i64 123, i64 456>
+  %YEq = icmp eq <2 x i64> %Y, <i64 123, i64 456>
+  %either = select <2 x i1> %XEq, <2 x i1> <i1 true, i1 true>, <2 x i1> %YEq
+  %both = select <2 x i1> %XEq, <2 x i1> %YEq, <2 x i1> <i1 false, i1 false>
+  %cmp = icmp eq <2 x i64> %X, %Y
+  %equal = select <2 x i1> %either, <2 x i1> %both, <2 x i1> %cmp
+  ret <2 x i1> %equal
+}
+
+define i1 @icmp_equality_test_commute_icmp(i64 %X, i64 %Y) {
+; CHECK-LABEL: @icmp_equality_test_commute_icmp(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i64 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    ret i1 [[CMP]]
+;
+entry:
+  %XEq = icmp eq i64 0, %X
+  %YEq = icmp eq i64 0, %Y
+  %either = select i1 %XEq, i1 true, i1 %YEq
+  %both = select i1 %XEq, i1 %YEq, i1 false
+  %cmp = icmp eq i64 %Y, %X
+  %equal = select i1 %either, i1 %both, i1 %cmp
+  ret i1 %equal
+}
+
+define i1 @icmp_equality_test_commute_select1(i64 %X, i64 %Y) {
+; CHECK-LABEL: @icmp_equality_test_commute_select1(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i64 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    ret i1 [[CMP]]
+;
+entry:
+  %XEq = icmp eq i64 %X, 0
+  %YEq = icmp eq i64 %Y, 0
+  %either = select i1 %YEq, i1 true, i1 %XEq
+  %both = select i1 %XEq, i1 %YEq, i1 false
+  %cmp = icmp eq i64 %X, %Y
+  %equal = select i1 %either, i1 %both, i1 %cmp
+  ret i1 %equal
+}
+
+define i1 @icmp_equality_test_commute_select2(i64 %X, i64 %Y) {
+; CHECK-LABEL: @icmp_equality_test_commute_select2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i64 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    ret i1 [[CMP]]
+;
+entry:
+  %XEq = icmp eq i64 %X, 0
+  %YEq = icmp eq i64 %Y, 0
+  %either = select i1 %XEq, i1 true, i1 %YEq
+  %both = select i1 %YEq, i1 %XEq, i1 false
+  %cmp = icmp eq i64 %X, %Y
+  %equal = select i1 %either, i1 %both, i1 %cmp
+  ret i1 %equal
+}
+
+; Negative tests below
+
+define i1 @icmp_equality_test_wrong_constant(i64 %X, i64 %Y) {
+; CHECK-LABEL: @icmp_equality_test_wrong_constant(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[XEQ:%.*]] = icmp eq i64 [[X:%.*]], 0
+; CHECK-NEXT:    [[YEQ:%.*]] = icmp eq i64 [[Y:%.*]], 999
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i64 [[X]], [[Y]]
+; CHECK-NEXT:    [[NOT_YEQ:%.*]] = xor i1 [[YEQ]], true
+; CHECK-NEXT:    [[BOTH:%.*]] = select i1 [[NOT_YEQ]], i1 [[CMP]], i1 false
+; CHECK-NEXT:    [[EQUAL:%.*]] = select i1 [[XEQ]], i1 [[YEQ]], i1 [[BOTH]]
+; CHECK-NEXT:    ret i1 [[EQUAL]]
+;
+entry:
+  %XEq = icmp eq i64 %X, 0
+  %YEq = icmp eq i64 %Y, 999
+  %either = select i1 %XEq, i1 true, i1 %YEq
+  %both = select i1 %XEq, i1 %YEq, i1 false
+  %cmp = icmp eq i64 %X, %Y
+  %equal = select i1 %either, i1 %both, i1 %cmp
+  ret i1 %equal
+}
+
+define i1 @icmp_equality_test_wrong_either(i64 %X, i64 %Y) {
+; CHECK-LABEL: @icmp_equality_test_wrong_either(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[XEQ_NOT:%.*]] = icmp eq i64 [[X:%.*]], 0
+; CHECK-NEXT:    [[YEQ:%.*]] = icmp eq i64 [[Y:%.*]], 999
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i64 [[Y]], 0
+; CHECK-NEXT:    [[BOTH:%.*]] = or i1 [[YEQ]], [[CMP]]
+; CHECK-NEXT:    [[EQUAL:%.*]] = select i1 [[XEQ_NOT]], i1 [[BOTH]], i1 false
+; CHECK-NEXT:    ret i1 [[EQUAL]]
+;
+entry:
+  %XEq = icmp eq i64 %X, 0
+  %YEq = icmp eq i64 %Y, 999
+  %either = select i1 %XEq, i1 %YEq, i1 true
+  %both = select i1 %XEq, i1 %YEq, i1 false
+  %cmp = icmp eq i64 %X, %Y
+  %equal = select i1 %either, i1 %both, i1 %cmp
+  ret i1 %equal
+}
+
+define i1 @icmp_equality_test_wrong_both(i64 %X, i64 %Y) {
+; CHECK-LABEL: @icmp_equality_test_wrong_both(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[XEQ:%.*]] = icmp ne i64 [[X:%.*]], 0
+; CHECK-NEXT:    [[YEQ:%.*]] = icmp eq i64 [[Y:%.*]], 0
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i64 [[X]], [[Y]]
+; CHECK-NEXT:    [[BOTH:%.*]] = select i1 [[YEQ]], i1 true, i1 [[CMP]]
+; CHECK-NEXT:    [[EQUAL:%.*]] = select i1 [[XEQ]], i1 [[BOTH]], i1 false
+; CHECK-NEXT:    ret i1 [[EQUAL]]
+;
+entry:
+  %XEq = icmp eq i64 %X, 0
+  %YEq = icmp eq i64 %Y, 0
+  %either = select i1 %XEq, i1 true, i1 %YEq
+  %both = select i1 %XEq, i1 false, i1 %YEq
+  %cmp = icmp eq i64 %X, %Y
+  %equal = select i1 %either, i1 %both, i1 %cmp
+  ret i1 %equal
+}
+
+define i1 @icmp_equality_test_wrong_cmp(i64 %X, i64 %Y) {
+; CHECK-LABEL: @icmp_equality_test_wrong_cmp(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[XEQ:%.*]] = icmp eq i64 [[X:%.*]], 0
+; CHECK-NEXT:    [[YEQ:%.*]] = icmp eq i64 [[Y:%.*]], 0
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i64 [[X]], 999
+; CHECK-NEXT:    [[NOT_YEQ:%.*]] = xor i1 [[YEQ]], true
+; CHECK-NEXT:    [[BOTH:%.*]] = select i1 [[NOT_YEQ]], i1 [[CMP]], i1 false
+; CHECK-NEXT:    [[EQUAL:%.*]] = select i1 [[XEQ]], i1 [[YEQ]], i1 [[BOTH]]
+; CHECK-NEXT:    ret i1 [[EQUAL]]
+;
+entry:
+  %XEq = icmp eq i64 %X, 0
+  %YEq = icmp eq i64 %Y, 0
+  %either = select i1 %XEq, i1 true, i1 %YEq
+  %both = select i1 %XEq, i1 %YEq, i1 false
+  %cmp = icmp eq i64 %X, 999
+  %equal = select i1 %either, i1 %both, i1 %cmp
+  ret i1 %equal
+}
+
+define i1 @icmp_equality_test_wrong_equal(i64 %X, i64 %Y) {
+; CHECK-LABEL: @icmp_equality_test_wrong_equal(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[XEQ:%.*]] = icmp eq i64 [[X:%.*]], 0
+; CHECK-NEXT:    [[YEQ:%.*]] = icmp eq i64 [[Y:%.*]], 0
+; CHECK-NEXT:    [[EITHER:%.*]] = select i1 [[XEQ]], i1 true, i1 [[YEQ]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i64 [[X]], [[Y]]
+; CHECK-NEXT:    [[EQUAL:%.*]] = select i1 [[EITHER]], i1 [[CMP]], i1 false
+; CHECK-NEXT:    ret i1 [[EQUAL]]
+;
+entry:
+  %XEq = icmp eq i64 %X, 0
+  %YEq = icmp eq i64 %Y, 0
+  %either = select i1 %XEq, i1 true, i1 %YEq
+  %both = select i1 %XEq, i1 %YEq, i1 false
+  %cmp = icmp eq i64 %X, %Y
+  %equal = select i1 %either, i1 %cmp, i1 %both
+  ret i1 %equal
+}

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Sep 14, 2024
@goldsteinn
Copy link
Contributor

This is really really specific, if it shows up in realworld code I'm not opposed to getting it in, but is there some generalization of the pattern we can make?

@citymarina
Copy link
Contributor Author

This is really really specific, if it shows up in realworld code I'm not opposed to getting it in, but is there some generalization of the pattern we can make?

Oh, interesting. I was worried that there would be concern about having overgeneralized already (my original unpublished version just checked for constant 0, with no commutativity, etc.). I've tried to relax as many of the constraints as I could see; do you have anything specific in mind about how to take it further?

@goldsteinn
Copy link
Contributor

This is really really specific, if it shows up in realworld code I'm not opposed to getting it in, but is there some generalization of the pattern we can make?

Oh, interesting. I was worried that there would be concern about having overgeneralized already (my original unpublished version just checked for constant 0, with no commutativity, etc.). I've tried to relax as many of the constraints as I could see; do you have anything specific in mind about how to take it further?

I don't really see any obvious generalization either. If its backed up by a realworld use-case its probably fine.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like InstCombine canonicalizes this to something like (X == Z) ? (Y == Z) : !(Y == Z) && X == Y), see https://alive2.llvm.org/ce/z/XY_HMX. Shouldn't we be matching that pattern instead?

@citymarina
Copy link
Contributor Author

It looks like InstCombine canonicalizes this to something like (X == Z) ? (Y == Z) : !(Y == Z) && X == Y), see https://alive2.llvm.org/ce/z/XY_HMX. Shouldn't we be matching that pattern instead?

I am new in this area and unfamiliar with the conventions. My thought process was that matching something closer to the Swift-generated IR would better express the intent of this patch, and also I wasn't sure if I could rely on the altered form remaining stable over time. But if you think it would be better then I'm happy to update the patch.

@goldsteinn
Copy link
Contributor

It looks like InstCombine canonicalizes this to something like (X == Z) ? (Y == Z) : !(Y == Z) && X == Y), see https://alive2.llvm.org/ce/z/XY_HMX. Shouldn't we be matching that pattern instead?

I am new in this area and unfamiliar with the conventions. My thought process was that matching something closer to the Swift-generated IR would better express the intent of this patch, and also I wasn't sure if I could rely on the altered form remaining stable over time. But if you think it would be better then I'm happy to update the patch.

Typically canonical patterns are what remain stable longer. They aren't folds that meaningful change the instructions, just standardize them in a way so that future matching code only needs to worry about a single iteration.
You can see more about it: https://llvm.org/docs/InstCombineContributorGuide.html#canonicalization-and-target-independence

@citymarina
Copy link
Contributor Author

It looks like InstCombine canonicalizes this to something like (X == Z) ? (Y == Z) : !(Y == Z) && X == Y), see https://alive2.llvm.org/ce/z/XY_HMX. Shouldn't we be matching that pattern instead?

I am new in this area and unfamiliar with the conventions. My thought process was that matching something closer to the Swift-generated IR would better express the intent of this patch, and also I wasn't sure if I could rely on the altered form remaining stable over time. But if you think it would be better then I'm happy to update the patch.

Typically canonical patterns are what remain stable longer. They aren't folds that meaningful change the instructions, just standardize them in a way so that future matching code only needs to worry about a single iteration. You can see more about it: https://llvm.org/docs/InstCombineContributorGuide.html#canonicalization-and-target-independence

That makes sense. I guess the missing piece for me was that I didn't realize (X == Z) ? (Y == Z) : !(Y == Z) && X == Y) was in fact a canonicalization. I'll work on an update.

This corresponds to the canonicalized form of some logic that was
seen in Swift-generated code for comparing optional pointers:
`(X==Z || Y==Z) ? (X==Z && Y==Z) : X==Y --> X==Y`
where `Z` was the constant `0`.

https://alive2.llvm.org/ce/z/J_3aa9
@citymarina
Copy link
Contributor Author

A nice result of matching on the canonical form is that there is no longer an ordering dependency, so I've removed the This needs to happen before ... comment.

@citymarina citymarina changed the title [InstCombine] Fold (X==Z || Y==Z) ? (X==Z && Y==Z) : X==Y --> X==Y [InstCombine] Fold (X==Z) ? (Y==Z) : (!(Y==Z) && X==Y) --> X==Y Sep 20, 2024
@citymarina
Copy link
Contributor Author

Ping? Thanks.

@goldsteinn
Copy link
Contributor

This could (but doesn't) work for cases where the compare ops are inverted for example:
https://alive2.llvm.org/ce/z/6BD9mt

Since this is motivated by a pretty specific case where we always get equal expressions I'm not sure this is an issue worth fixing given the extra code complexity required to do so.

Copy link
Contributor

@goldsteinn goldsteinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Wait a day or so to push to give others a chance to take a look.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please update the alive2 proof in the PR description? Otherwise this looks good to me.

@fhahn
Copy link
Contributor

fhahn commented Oct 3, 2024

Looks like the alive2 proof in the description has been updated, thanks!

@fhahn fhahn merged commit d0d12fc into llvm:main Oct 3, 2024
8 checks passed
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
…#108619)

This corresponds to the canonicalized form of some logic that was
seen in Swift-generated code for comparing optional pointers:
`(X==Z || Y==Z) ? (X==Z && Y==Z) : X==Y --> X==Y`
where `Z` was the constant `0`.

https://alive2.llvm.org/ce/z/J_3aa9
citymarina added a commit to citymarina/llvm-project that referenced this pull request Oct 7, 2024
…#108619)

This corresponds to the canonicalized form of some logic that was
seen in Swift-generated code for comparing optional pointers:
`(X==Z || Y==Z) ? (X==Z && Y==Z) : X==Y --> X==Y`
where `Z` was the constant `0`.

https://alive2.llvm.org/ce/z/J_3aa9
(cherry picked from commit d0d12fc)
@citymarina citymarina deleted the equality-test branch November 26, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants