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]: Handle known negative variables with KnownBits #89682

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AtariDreams
Copy link
Contributor

No description provided.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 22, 2024

@llvm/pr-subscribers-llvm-transforms

Author: AtariDreams (AtariDreams)

Changes

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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp (+7-1)
  • (modified) llvm/test/Transforms/InstCombine/udiv-simplify.ll (+12)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
index 4ed4c36e21e016..f03c31e613d601 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
@@ -1491,12 +1491,18 @@ Instruction *InstCombinerImpl::visitUDiv(BinaryOperator &I) {
   }
 
   // Op0 / C where C is large (negative) --> zext (Op0 >= C)
-  // TODO: Could use isKnownNegative() to handle non-constant values.
   Type *Ty = I.getType();
   if (match(Op1, m_Negative())) {
     Value *Cmp = Builder.CreateICmpUGE(Op0, Op1);
     return CastInst::CreateZExtOrBitCast(Cmp, Ty);
   }
+
+  KnownBits Known = computeKnownBits(Op1, 0, &I);
+  if (Known.isNegative()) {
+    Value *Cmp = Builder.CreateICmpUGE(Op0, Op1);
+    return CastInst::CreateZExtOrBitCast(Cmp, Ty);
+  }
+
   // Op0 / (sext i1 X) --> zext (Op0 == -1) (if X is 0, the div is undefined)
   if (match(Op1, m_SExt(m_Value(X))) && X->getType()->isIntOrIntVectorTy(1)) {
     Value *Cmp = Builder.CreateICmpEQ(Op0, ConstantInt::getAllOnesValue(Ty));
diff --git a/llvm/test/Transforms/InstCombine/udiv-simplify.ll b/llvm/test/Transforms/InstCombine/udiv-simplify.ll
index bd6e5efc05f187..82532e3e6ff8c9 100644
--- a/llvm/test/Transforms/InstCombine/udiv-simplify.ll
+++ b/llvm/test/Transforms/InstCombine/udiv-simplify.ll
@@ -98,6 +98,18 @@ define i8 @udiv_demanded_low_bits_set(i8 %a) {
   ret i8 %u
 }
 
+define i8 @udiv_large_value(i8 %a) {
+; CHECK-LABEL: @udiv_large_value(
+; CHECK-NEXT:    [[TMP1:%.*]] = and i8 [[A:%.*]], -76
+; CHECK-NEXT:    [[TMP2:%.*]] = icmp eq i8 [[TMP1]], -76
+; CHECK-NEXT:    [[U:%.*]] = zext i1 [[TMP2]] to i8
+; CHECK-NEXT:    ret i8 [[U]]
+;
+  %o = or i8 %a, 180
+  %u = udiv i8 %a, %o
+  ret i8 %u
+}
+
 ; This can't divide evenly, so it is poison.
 
 define i8 @udiv_exact_demanded_low_bits_set(i8 %a) {

Type *Ty = I.getType();
if (match(Op1, m_Negative())) {
Value *Cmp = Builder.CreateICmpUGE(Op0, Op1);
return CastInst::CreateZExtOrBitCast(Cmp, Ty);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The m_Negative code is now unnecessary.

; CHECK-NEXT: [[U:%.*]] = zext i1 [[TMP2]] to i8
; CHECK-NEXT: ret i8 [[U]]
;
%o = or i8 %a, 180
Copy link
Contributor

Choose a reason for hiding this comment

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

imo make the constant 128 so the verification of this transform is the most general case.

@nikic
Copy link
Contributor

nikic commented Apr 23, 2024

@dtcxzyw Does this have any impact? If not I'm going to drop the TODO instead.

@goldsteinn
Copy link
Contributor

@dtcxzyw Does this have any impact? If not I'm going to drop the TODO instead.

+1

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

4 participants