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 one-use limitation to box multiply fold #72876

Merged
merged 5 commits into from
Dec 4, 2023

Conversation

shaojingzhi
Copy link
Contributor

Check the operands of I are used in no more than one place, which can not be deleted, cause a mul instruction has far more weight than add and shl instruction in IR, thus this method cannot achieve the goal of simplifying instructions, just return null.

Add a situation that mul cannot be replaced by add and shl.
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 20, 2023

@llvm/pr-subscribers-llvm-transforms

Author: None (shaojingzhi)

Changes

Check the operands of I are used in no more than one place, which can not be deleted, cause a mul instruction has far more weight than add and shl instruction in IR, thus this method cannot achieve the goal of simplifying instructions, just return null.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp (+8)
  • (modified) llvm/test/Transforms/InstCombine/mul_full_64.ll (+4-1)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
index 90b1c133461a408..5b82c3179792f64 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
@@ -1405,6 +1405,14 @@ static Instruction *foldBoxMultiply(BinaryOperator &I) {
   // ResLo = (CrossSum << HalfBits) + (YLo * XLo)
   Value *XLo, *YLo;
   Value *CrossSum;
+  
+  // Checking the operands of I is used in no more than one place,
+  // which can not be deleted, cause a mul instruction has far more weight than
+  // add and shl instruction in IR, thus this method cannot achieve the goal of
+  // simplifying instructions, just return null.
+  if ((!I.getOperand(0)->hasOneUser() || !I.getOperand(1)->hasOneUser()))
+    return nullptr;
+
   if (!match(&I, m_c_Add(m_Shl(m_Value(CrossSum), m_SpecificInt(HalfBits)),
                          m_Mul(m_Value(YLo), m_Value(XLo)))))
     return nullptr;
diff --git a/llvm/test/Transforms/InstCombine/mul_full_64.ll b/llvm/test/Transforms/InstCombine/mul_full_64.ll
index 8a57b548cd14b9e..5c57270fb147db8 100644
--- a/llvm/test/Transforms/InstCombine/mul_full_64.ll
+++ b/llvm/test/Transforms/InstCombine/mul_full_64.ll
@@ -177,6 +177,7 @@ define i64 @mul_full_64_variant2(i64 %a, i64 %b, ptr nocapture %rhi) {
   ret i64 %add27
 }
 
+; Negative test case for mul_fold function: MUL7 is used in more than one place
 define i64 @mul_full_64_variant3(i64 %a, i64 %b, ptr nocapture %rhi) {
 ; CHECK-LABEL: @mul_full_64_variant3(
 ; CHECK-NEXT:    [[CONV:%.*]] = and i64 [[A:%.*]], 4294967295
@@ -196,7 +197,9 @@ define i64 @mul_full_64_variant3(i64 %a, i64 %b, ptr nocapture %rhi) {
 ; CHECK-NEXT:    [[SHR_I:%.*]] = lshr i64 [[ADD15]], 32
 ; CHECK-NEXT:    [[ADD17:%.*]] = add i64 [[ADD10]], [[SHR_I]]
 ; CHECK-NEXT:    store i64 [[ADD17]], ptr [[RHI:%.*]], align 8
-; CHECK-NEXT:    [[ADD19:%.*]] = mul i64 [[A]], [[B]]
+; CHECK-NEXT:    [[ADD18:%.*]] = add i64 [[MUL6]], [[MUL5]]
+; CHECK-NEXT:    [[SHL:%.*]] = shl i64 [[ADD18]], 32
+; CHECK-NEXT:    [[ADD19:%.*]] = add i64 [[SHL]], [[MUL7]]
 ; CHECK-NEXT:    ret i64 [[ADD19]]
 ;
   %conv = and i64 %a, 4294967295

@nikic nikic changed the title Add a situattion that mul instruction should not be replaced. [InstCombine] Add one-use limitation to box multiply fold Nov 20, 2023
@dtcxzyw dtcxzyw requested review from chfast and vfdff November 21, 2023 02:34
Co-authored-by: Yingwei Zheng <dtcxzyw@qq.com>
Copy link

github-actions bot commented Nov 22, 2023

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

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.

@@ -1405,8 +1405,13 @@ static Instruction *foldBoxMultiply(BinaryOperator &I) {
// ResLo = (CrossSum << HalfBits) + (YLo * XLo)
Value *XLo, *YLo;
Value *CrossSum;

Copy link
Member

Choose a reason for hiding this comment

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

Please drop redundant spaces here.

Modify annotation.

Co-authored-by: Nikita Popov <github@npopov.com>
Add test case to show shl does not need hasOneUse constraint
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

@nikic nikic merged commit 9a99a1a into llvm:main Dec 4, 2023
3 checks passed
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

6 participants