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] Add ctpop(A | B) + ctpop(A & B) -> ctpop(A) + ctpop(B) #79089

Closed
wants to merge 2 commits into from

Conversation

AtariDreams
Copy link
Contributor

No description provided.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 23, 2024

@llvm/pr-subscribers-llvm-transforms

Author: AtariDreams (AtariDreams)

Changes

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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp (+19)
  • (modified) llvm/test/Transforms/InstCombine/ctpop.ll (+15)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
index 8a00b75a1f74042..d8ef770d6fcef04 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
@@ -1718,6 +1718,25 @@ Instruction *InstCombinerImpl::visitAdd(BinaryOperator &I) {
         Builder.CreateIntrinsic(Intrinsic::umax, {I.getType()}, {A, B}));
   }
 
+  // ctpop(A | B) + ctpop(A & B) -> ctpop(A) + ctpop(B)
+  if ((match(LHS, m_OneUse(m_Intrinsic<Intrinsic::ctpop>(
+                      m_And(m_Value(A), m_Value(B))))) &&
+       (match(RHS, m_OneUse(m_Intrinsic<Intrinsic::ctpop>(
+                       m_Or(m_Specific(A), m_Specific(B))))) ||
+        match(RHS, m_OneUse(m_Intrinsic<Intrinsic::ctpop>(
+                       m_Or(m_Specific(B), m_Specific(A))))))) ||
+      (match(LHS, m_OneUse(m_Intrinsic<Intrinsic::ctpop>(
+                      m_Or(m_Value(A), m_Value(B))))) &&
+       (match(RHS, m_OneUse(m_Intrinsic<Intrinsic::ctpop>(
+                       m_And(m_Specific(A), m_Specific(B))))) ||
+        match(RHS, m_OneUse(m_Intrinsic<Intrinsic::ctpop>(
+                       m_And(m_Specific(B), m_Specific(A)))))))) {
+    return replaceInstUsesWith(
+        I, Builder.CreateAdd(
+               Builder.CreateIntrinsic(Intrinsic::ctpop, {A->getType()}, {A}),
+               Builder.CreateIntrinsic(Intrinsic::ctpop, {B->getType()}, {B})));
+  }
+
   // ctpop(A) + ctpop(B) => ctpop(A | B) if A and B have no bits set in common.
   if (match(LHS, m_OneUse(m_Intrinsic<Intrinsic::ctpop>(m_Value(A)))) &&
       match(RHS, m_OneUse(m_Intrinsic<Intrinsic::ctpop>(m_Value(B)))) &&
diff --git a/llvm/test/Transforms/InstCombine/ctpop.ll b/llvm/test/Transforms/InstCombine/ctpop.ll
index dcea5fa87479eb4..6dd8faf65a6ff8c 100644
--- a/llvm/test/Transforms/InstCombine/ctpop.ll
+++ b/llvm/test/Transforms/InstCombine/ctpop.ll
@@ -264,6 +264,21 @@ define <2 x i32> @ctpop_add_no_common_bits_vec_use2(<2 x i32> %a, <2 x i32> %b,
   ret <2 x i32> %res
 }
 
+define i32 @ctpop_and_or_combine(i32 %a, i32 %b) {
+; CHECK-LABEL: @ctpop_and_or_combine(
+; CHECK-NEXT:    [[TMP1:%.*]] = call i32 @llvm.ctpop.i32(i32 [[A:%.*]]), !range [[RNG1]]
+; CHECK-NEXT:    [[TMP2:%.*]] = call i32 @llvm.ctpop.i32(i32 [[B:%.*]]), !range [[RNG1]]
+; CHECK-NEXT:    [[RES:%.*]] = add nuw nsw i32 [[TMP1]], [[TMP2]]
+; CHECK-NEXT:    ret i32 [[RES]]
+;
+  %and = and i32 %a, %b
+  %ctpop1 = tail call i32 @llvm.ctpop.i32(i32 %and)
+  %or = or i32 %a, %b
+  %ctpop2 = tail call i32 @llvm.ctpop.i32(i32 %or)
+  %res = add nuw nsw i32 %ctpop1, %ctpop2
+  ret i32 %res
+}
+
 define i8 @ctpop_rotate_left(i8 %a, i8 %amt)  {
 ; CHECK-LABEL: @ctpop_rotate_left(
 ; CHECK-NEXT:    [[RES:%.*]] = tail call i8 @llvm.ctpop.i8(i8 [[A:%.*]]), !range [[RNG0]]

Copy link

github-actions bot commented Jan 23, 2024

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

@AtariDreams
Copy link
Contributor Author

@dtcxzyw Done!

@DavidSpickett
Copy link
Collaborator

(since you asked about this on Discord)

If you click through to the results and click a whole bunch more (it's not super easy to find), you'll get to the end of the log and see:

MT: command "C:\Program Files (x86)\Windows Kits\10\bin\10.0.19041.0\x64\mt.exe /nologo /manifest unittests\TargetParser\TargetParserTests.exe.manifest /outputresource:unittests\TargetParser\TargetParserTests.exe;#1" failed (exit code 0x1f) with the following output:
mt.exe : general error c101008d: Failed to write the updated manifest to the resource of file "unittests\TargetParser\TargetParserTests.exe". Operation did not complete successfully because the file contains a virus or potentially unwanted software.

Which is not related to your patch. Ideally you'd get a clean run at some point, maybe by rebasing once the patch is approved, but if that's not possible maybe your reviewers will allow a merge without a Windows CI run.

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Jan 30, 2024
@dtcxzyw
Copy link
Member

dtcxzyw commented Jan 30, 2024

@AtariDreams Does this pattern exist in some real-world applications?

@AtariDreams
Copy link
Contributor Author

@AtariDreams Does this pattern exist in some real-world applications?

Given the popcount(A|B) does exist, it makes sense that popcount(A&B) + popcount(A|B) exists.

@dtcxzyw
Copy link
Member

dtcxzyw commented Feb 20, 2024

The implementation LGTM. But I am sorry I cannot give the approval unless you can prove that it benefits some real-world applications.

There are an infinite number of atomic simplifications. But obviously you cannot handle all of them in LLVM.

I am not willing to waste my time reviewing these valueless patches. I recommend you look at issues created by @XChy, which are extracted from real-world applications.
https://github.com/llvm/llvm-project/issues/created_by/XChy

@dtcxzyw dtcxzyw removed their request for review February 20, 2024 07:55
@nikic
Copy link
Contributor

nikic commented Feb 20, 2024

I agree with @dtcxzyw. The pattern here is simple enough that we could accept it without knowing about a specific use-case, but at the same time I have a really hard time imaging where this could possibly come up. I'm going to decline it for now.

@nikic nikic closed this Feb 20, 2024
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

5 participants