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] Change (add x, c) to (xor x, c) #75129

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

Peter9606
Copy link
Contributor

@Peter9606 Peter9606 commented Dec 12, 2023

Change (add x, c) to (xor x, c) iff c is constant and c equals the top bit of the demanded bits.
Alive2: https://alive2.llvm.org/ce/z/DKmkwF

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 12, 2023

@llvm/pr-subscribers-llvm-transforms

Author: Fujun Han (Peter9606)

Changes

Simplify a complex OR to XOR

Following pattern:
  (or (and x, half_c1), c3), (and x, c2))
  IFF
    c1, c2, c3 is constant
    c1 is pow2
    c2 < c1
    c3 == (c1 - 1) ^ c2
    half_c1 = (lshr c1, 1)
    (c1 >> 1) & c3 == (c1 >> 1)
    x is known to be less than c1
can be simplified into:
  (xor x, half_c)

Proof: https://alive2.llvm.org/ce/z/Lfax3w


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp (+21-1)
  • (added) llvm/test/Transforms/InstCombine/or-and-add-constant-constant-and-constant.ll (+36)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 6002f599ca71ab..67ec4a1608169a 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -3343,6 +3343,27 @@ Instruction *InstCombinerImpl::visitOr(BinaryOperator &I) {
   if (Instruction *X = foldComplexAndOrPatterns(I, Builder))
     return X;
 
+  // Change (or (and (add x, half_c1), c3), (and x, c2)) to (xor x, half_c1),
+  // iff c1, c2, c3 is constant, and half_c1 = (lshr c1, 1),  and c1 is pow2,
+  // and c2 < c1, and c3 == (c1 - 1) ^ c2, and (c1 >> 1) & c3 == (c1 >> 1) and x
+  // is known to be less than c1.
+  Type *Ty = I.getType();
+  if (Ty->isIntegerTy()) {
+    Value *X = nullptr;
+    const APInt *HalfC1 = nullptr, *C2 = nullptr, *C3 = nullptr;
+    if (match(&I,
+              m_c_Or(m_c_And(m_c_Add(m_Value(X), m_APInt(HalfC1)), m_APInt(C3)),
+                     m_c_And(m_Value(X), m_APInt(C2))))) {
+      const APInt C1 = HalfC1->shl(1);
+      KnownBits KnownX = computeKnownBits(X, 0, nullptr);
+      if (C1.isPowerOf2() && C2->ult(C1) && (*C3 == (*C2 ^ (C1 - 1))) &&
+          ((*HalfC1 & *C3) == *HalfC1) && KnownX.getMaxValue().ult(C1)) {
+        Value *Xor = Builder.CreateXor(X, ConstantInt::get(Ty, *HalfC1));
+        return replaceInstUsesWith(I, Xor);
+      }
+    }
+  }
+
   // (A&B)|(A&C) -> A&(B|C) etc
   if (Value *V = foldUsingDistributiveLaws(I))
     return replaceInstUsesWith(I, V);
@@ -3351,7 +3372,6 @@ Instruction *InstCombinerImpl::visitOr(BinaryOperator &I) {
     return replaceInstUsesWith(I, V);
 
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
-  Type *Ty = I.getType();
   if (Ty->isIntOrIntVectorTy(1)) {
     if (auto *SI0 = dyn_cast<SelectInst>(Op0)) {
       if (auto *R =
diff --git a/llvm/test/Transforms/InstCombine/or-and-add-constant-constant-and-constant.ll b/llvm/test/Transforms/InstCombine/or-and-add-constant-constant-and-constant.ll
new file mode 100644
index 00000000000000..3e48f3c3f331b9
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/or-and-add-constant-constant-and-constant.ll
@@ -0,0 +1,36 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
+; Pattern:
+;   (or (and (add x, half_c1), c3), (and x, c2))
+; IFF:
+;   c1, c2, c3 is constant
+;   c1 is pow2
+;   c2 < c1
+;   c3 == (c1 - 1) ^ c2
+;   half_c1 == (lshr c1, 1)
+;   (c1 >> 1) & c3 == (c1 >> 1)
+;   x is known to be less than c1
+; Could be transformed into:
+;   (xor x, half_c1)
+; Proof: https://alive2.llvm.org/ce/z/Lfax3w
+
+define i16 @or_and_add_and() {
+; CHECK-LABEL: @or_and_add_and(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[X:%.*]] = call i16 @dummy(), !range [[RNG0:![0-9]+]]
+; CHECK-NEXT:    [[OR:%.*]] = xor i16 [[X]], 32
+; CHECK-NEXT:    ret i16 [[OR]]
+;
+entry:
+  %x = call i16 @dummy(), !range !0
+  %add = add i16 32, %x
+  %and1 = and i16 %add, 48
+  %and2 = and i16 %x, 15
+  %or = or i16 %and1, %and2
+  ret i16 %or
+}
+
+declare i16 @dummy()
+
+!0 = !{i16 0, i16 64}

Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

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

This pattern looks complex. Does it come from a real-world application?

@dtcxzyw dtcxzyw changed the title Fujun.han/instcombine or to xor [InstCombine] Simplify a complex OR to XOR Dec 12, 2023
@Peter9606
Copy link
Contributor Author

This pattern looks complex. Does it come from a real-world application?

Yes, it does.

@Peter9606 Peter9606 force-pushed the fujun.han/instcombine-or-to-xor branch from a5420c8 to 3cdf54e Compare December 12, 2023 06:46
@bcl5980
Copy link
Contributor

bcl5980 commented Dec 12, 2023

Precommit test First, then second commit is code change + test diff.
It looks your test is the second commit.

@Peter9606 Peter9606 force-pushed the fujun.han/instcombine-or-to-xor branch 2 times, most recently from 0a31b4a to 0f35e3b Compare December 12, 2023 13:59
@Peter9606
Copy link
Contributor Author

Precommit test First, then second commit is code change + test diff. It looks your test is the second commit.

You are right, fixed! Thanks!

@bcl5980
Copy link
Contributor

bcl5980 commented Dec 12, 2023

We may need to rewrite the conditions.
https://alive2.llvm.org/ce/z/-ct9Wt

Not all cases can be catched for now.

@Peter9606 Peter9606 force-pushed the fujun.han/instcombine-or-to-xor branch from 0f35e3b to bba2fc6 Compare December 13, 2023 01:57
@topperc
Copy link
Collaborator

topperc commented Dec 13, 2023

I think (and (add x, 32), 48) can be replaced with (and (xor x, 32), 48). Is that optimization alone enough to trigger the rest?

@Peter9606
Copy link
Contributor Author

I think (and (add x, 32), 48) can be replaced with (and (xor x, 32), 48). Is that optimization alone enough to trigger the rest?

Just did a quick verify, your sugguestion works. At least for the case pointed by @bcl5980 in comment, it can be simplified to XOR now.

@Peter9606
Copy link
Contributor Author

I think (and (add x, 32), 48) can be replaced with (and (xor x, 32), 48). Is that optimization alone enough to trigger the rest?

@topperc If I understand correctlly, your suggestion can be formalized as https://alive2.llvm.org/ce/z/aoJPsn, right?

@nikic
Copy link
Contributor

nikic commented Dec 13, 2023

@Peter9606 We'd probably approach that as a demanded bits transform (

). Convert X + C to X ^ C if C is equal to the top bit of the DemandedMask.

@Peter9606 Peter9606 force-pushed the fujun.han/instcombine-or-to-xor branch 2 times, most recently from a553aa3 to 5bfa471 Compare December 14, 2023 06:00
@Peter9606 Peter9606 changed the title [InstCombine] Simplify a complex OR to XOR [InstCombine] Change (add x, c) to (xor x, c) Dec 14, 2023
@Peter9606
Copy link
Contributor Author

@Peter9606 We'd probably approach that as a demanded bits transform (

). Convert X + C to X ^ C if C is equal to the top bit of the DemandedMask.

Thanks for your detail guidance. @nikic
PR is updated!

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.

Looks reasonable to me.

llvm/test/Transforms/InstCombine/and.ll Show resolved Hide resolved
@bcl5980
Copy link
Contributor

bcl5980 commented Dec 14, 2023

Add a vector case?

Peter Han added 2 commits December 14, 2023 19:55
Signed-off-by: Peter Han <fujun.han@iluvatar.com>
…equals the top bit of the demanded bits.

Signed-off-by: Peter Han <fujun.han@iluvatar.com>
@Peter9606 Peter9606 force-pushed the fujun.han/instcombine-or-to-xor branch from 5bfa471 to 2295866 Compare December 14, 2023 12:00
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.

LGTM

Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

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

LGTM. I will merge this PR after the CI is complete.

@dtcxzyw dtcxzyw merged commit 7be5dab into llvm:main Dec 14, 2023
4 checks passed
@jplehr
Copy link
Contributor

jplehr commented Dec 14, 2023

I see errors in our buildbot (https://lab.llvm.org/buildbot/#/builders/193/builds/43753) after this patch landed.

Reverting this patch locally resolves the errors.
If you need additional info / help for reproducing, please let me know.

@nikic
Copy link
Contributor

nikic commented Dec 14, 2023

Should be fixed by 9100228.

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.

None yet

7 participants