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 negation of unsigned div of non-negatives #84951

Conversation

antoniofrighetto
Copy link
Contributor

Let InstCombine carry out the following fold: sub 0, (udiv nneg X, nneg C) -> sdiv nneg X, -C.
Proofs: https://alive2.llvm.org/ce/z/9vyBvs.

Not sure if we need to go through assumptionsFor to update AC for the second assume.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 12, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Antonio Frighetto (antoniofrighetto)

Changes

Let InstCombine carry out the following fold: sub 0, (udiv nneg X, nneg C) -> sdiv nneg X, -C.
Proofs: https://alive2.llvm.org/ce/z/9vyBvs.

Not sure if we need to go through assumptionsFor to update AC for the second assume.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp (+25-1)
  • (modified) llvm/test/Transforms/InstCombine/sub.ll (+63)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
index aaf7184a5562cd..7ec55c6aef00d4 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
@@ -2062,6 +2062,26 @@ static Instruction *foldSubOfMinMax(BinaryOperator &I,
   return nullptr;
 }
 
+/// Fold `sub 0, (udiv nneg X, nneg C)` into `sdiv nneg X, -C`
+static Instruction *foldNegationOfUDivOfNonNegatives(BinaryOperator &I,
+                                                     InstCombinerImpl &IC) {
+  Value *RHS = I.getOperand(1);
+  Value *X, *C;
+
+  if (match(RHS, m_OneUse(m_UDiv(m_Value(X), m_Value(C))))) {
+    KnownBits KnownX = IC.computeKnownBits(X, 0, &I);
+    KnownBits KnownC = IC.computeKnownBits(C, 0, &I);
+    if (KnownX.isNonNegative() && KnownC.isNonNegative()) {
+      bool HasNSW = I.hasNoSignedWrap();
+      auto *NegC = HasNSW ? IC.Builder.CreateNSWNeg(C, C->getName() + ".neg")
+                          : IC.Builder.CreateNeg(C, C->getName() + ".neg");
+      return BinaryOperator::CreateSDiv(X, NegC);
+    }
+  }
+
+  return nullptr;
+}
+
 Instruction *InstCombinerImpl::visitSub(BinaryOperator &I) {
   if (Value *V = simplifySubInst(I.getOperand(0), I.getOperand(1),
                                  I.hasNoSignedWrap(), I.hasNoUnsignedWrap(),
@@ -2153,8 +2173,12 @@ Instruction *InstCombinerImpl::visitSub(BinaryOperator &I) {
                                         Op1, *this))
       return BinaryOperator::CreateAdd(NegOp1, Op0);
   }
-  if (IsNegation)
+  if (IsNegation) {
+    if (Instruction *Res = foldNegationOfUDivOfNonNegatives(I, *this))
+      return Res;
+
     return TryToNarrowDeduceFlags(); // Should have been handled in Negator!
+  }
 
   // (A*B)-(A*C) -> A*(B-C) etc
   if (Value *V = foldUsingDistributiveLaws(I))
diff --git a/llvm/test/Transforms/InstCombine/sub.ll b/llvm/test/Transforms/InstCombine/sub.ll
index 249b5673c8acfd..7cd1a0ea1646c7 100644
--- a/llvm/test/Transforms/InstCombine/sub.ll
+++ b/llvm/test/Transforms/InstCombine/sub.ll
@@ -2626,3 +2626,66 @@ define i8 @sub_of_adds_2xc(i8 %x, i8 %y) {
   %r = sub i8 %xc, %yc
   ret i8 %r
 }
+
+define i8 @test_neg_of_udiv_of_nonnegs(i8 %a, i8 %b) {
+; CHECK-LABEL: @test_neg_of_udiv_of_nonnegs(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[COND1:%.*]] = icmp sgt i8 [[A:%.*]], -1
+; CHECK-NEXT:    call void @llvm.assume(i1 [[COND1]])
+; CHECK-NEXT:    [[COND2:%.*]] = icmp sgt i8 [[B:%.*]], -1
+; CHECK-NEXT:    call void @llvm.assume(i1 [[COND2]])
+; CHECK-NEXT:    ret i8 0
+;
+entry:
+  %cond1 = icmp sgt i8 %a, -1
+  call void @llvm.assume(i1 %cond1)
+  %cond2 = icmp sgt i8 %b, -1
+  call void @llvm.assume(i1 %cond2)
+  %div = udiv i8 %a, %b
+  %neg = sub nuw i8 0, %div
+  ret i8 %neg
+}
+
+define i8 @test_neg_of_udiv_of_nonnegs_2(i8 %a, i8 %b) {
+; CHECK-LABEL: @test_neg_of_udiv_of_nonnegs_2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[COND1:%.*]] = icmp sgt i8 [[A:%.*]], -1
+; CHECK-NEXT:    call void @llvm.assume(i1 [[COND1]])
+; CHECK-NEXT:    [[COND2:%.*]] = icmp sgt i8 [[B:%.*]], -1
+; CHECK-NEXT:    call void @llvm.assume(i1 [[COND2]])
+; CHECK-NEXT:    [[B_NEG:%.*]] = sub nsw i8 0, [[B]]
+; CHECK-NEXT:    [[NEG:%.*]] = sdiv i8 [[A]], [[B_NEG]]
+; CHECK-NEXT:    ret i8 [[NEG]]
+;
+entry:
+  %cond1 = icmp sgt i8 %a, -1
+  call void @llvm.assume(i1 %cond1)
+  %cond2 = icmp sgt i8 %b, -1
+  call void @llvm.assume(i1 %cond2)
+  %div = udiv i8 %a, %b
+  %neg = sub nsw i8 0, %div
+  ret i8 %neg
+}
+
+define i8 @test_neg_of_udiv_of_nonnegs_3(i8 %a, i8 %b) {
+; CHECK-LABEL: @test_neg_of_udiv_of_nonnegs_3(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[COND1:%.*]] = icmp sgt i8 [[A:%.*]], -1
+; CHECK-NEXT:    call void @llvm.assume(i1 [[COND1]])
+; CHECK-NEXT:    [[COND2:%.*]] = icmp sgt i8 [[B:%.*]], -1
+; CHECK-NEXT:    call void @llvm.assume(i1 [[COND2]])
+; CHECK-NEXT:    [[B_NEG:%.*]] = sub nsw i8 0, [[B]]
+; CHECK-NEXT:    [[NEG:%.*]] = sdiv i8 [[A]], [[B_NEG]]
+; CHECK-NEXT:    ret i8 [[NEG]]
+;
+entry:
+  %cond1 = icmp sgt i8 %a, -1
+  call void @llvm.assume(i1 %cond1)
+  %cond2 = icmp sgt i8 %b, -1
+  call void @llvm.assume(i1 %cond2)
+  %div = udiv i8 %a, %b
+  %neg = sub i8 0, %div
+  ret i8 %neg
+}
+
+declare void @llvm.assume(i1)

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.

We only want to perform this fold is RHS is constant.

@@ -2062,6 +2062,26 @@ static Instruction *foldSubOfMinMax(BinaryOperator &I,
return nullptr;
}

/// Fold `sub 0, (udiv nneg X, nneg C)` into `sdiv nneg X, -C`
Copy link
Contributor

Choose a reason for hiding this comment

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

You can handle some additional cases if we have exact flag:
https://alive2.llvm.org/ce/z/mmpBoS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm not wrong, the second example should make sense if RHS is not a constant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so, in that case you aren't saving any instructions and basically just throwing out exact and doing udiv-> sdiv. Both of which alone are not profitable.

@antoniofrighetto antoniofrighetto force-pushed the feature/instcombine-negation-udiv-of-nonnegs branch from 8f02f11 to b28d3bb Compare March 13, 2024 15:03

const auto &SQ = IC.getSimplifyQuery().getWithInstruction(&I);
if (match(RHS, m_OneUse(m_UDiv(m_Value(X), m_Constant(C)))) &&
isKnownNonNegative(X, SQ) && isKnownNonNegative(C, SQ)) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add some tests with non-splat vector constants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like ValueTracking results are suboptimal for the splat one? (https://alive2.llvm.org/ce/z/ZzGqrF).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we look past bitcast when computing known bits for dom conditions.

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp Outdated Show resolved Hide resolved
@antoniofrighetto antoniofrighetto force-pushed the feature/instcombine-negation-udiv-of-nonnegs branch from b28d3bb to 173cd41 Compare March 13, 2024 16:25
llvm/test/Transforms/InstCombine/sub.ll Show resolved Hide resolved
llvm/test/Transforms/InstCombine/sub.ll Outdated Show resolved Hide resolved
llvm/test/Transforms/InstCombine/sub.ll Outdated Show resolved Hide resolved
llvm/test/Transforms/InstCombine/sub.ll Outdated Show resolved Hide resolved
dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Mar 14, 2024
@dtcxzyw
Copy link
Member

dtcxzyw commented Mar 14, 2024

Emm, this patch breaks div + rem pairs :(

@nikic
Copy link
Contributor

nikic commented Mar 14, 2024

Emm, this patch breaks div + rem pairs :(

This is a good point... Maybe it's not worth doing this transform.

@antoniofrighetto antoniofrighetto force-pushed the feature/instcombine-negation-udiv-of-nonnegs branch from 173cd41 to 5085035 Compare March 14, 2024 11:18
@antoniofrighetto
Copy link
Contributor Author

PR updated, not sure if we can still save something good out of this though.

Let InstCombine carry out the following fold:
`sub 0, (udiv nneg X, nneg C)` -> `sdiv nneg X, -C`.

Proofs: https://alive2.llvm.org/ce/z/T9zPod.
@antoniofrighetto antoniofrighetto force-pushed the feature/instcombine-negation-udiv-of-nonnegs branch from 5085035 to dbe6694 Compare March 14, 2024 11:58
@antoniofrighetto
Copy link
Contributor Author

Closing this, as turned out to be not profitable.

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