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

[SelectionDAG] Add more cases for UDIV and SDIV #86452

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AtariDreams
Copy link
Contributor

Also have UDIV just exit early if it can calculate that LHS is less than the RHS.

Also have UDIV just exit early if it can calculate that LHS is less than the RHS.
@llvmbot llvmbot added llvm:support llvm:SelectionDAG SelectionDAGISel as well labels Mar 24, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 24, 2024

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-llvm-support

Author: AtariDreams (AtariDreams)

Changes

Also have UDIV just exit early if it can calculate that LHS is less than the RHS.


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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+28-3)
  • (modified) llvm/lib/Support/KnownBits.cpp (+3-1)
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 0ab5142ab81676..5f80c7c7315ee4 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -5392,14 +5392,39 @@ bool SelectionDAG::isKnownNeverZero(SDValue Op, unsigned Depth) const {
       return true;
     break;
   }
-  case ISD::UDIV:
-  case ISD::SDIV:
+  case ISD::UDIV: {
     // div exact can only produce a zero if the dividend is zero.
-    // TODO: For udiv this is also true if Op1 u<= Op0
     if (Op->getFlags().hasExact())
       return isKnownNeverZero(Op.getOperand(0), Depth + 1);
+
+    // If Op1 <= Op0, then Op0 is at least 1, and therefore not 0.
+    KnownBits Op0 = computeKnownBits(Op.getOperand(0), Depth + 1);
+    KnownBits Op1 = computeKnownBits(Op.getOperand(1), Depth + 1);
+    std::optional<bool> uge = KnownBits::uge(Op0, Op1);
+    if (uge && *uge)
+      return true;
+
+    if (KnownBits::udiv(Op0, Op1).isNonZero())
+      return true;
     break;
+  }
+  case ISD::SDIV: {
+    // div exact can only produce a zero if the dividend is zero.
+    if (Op->getFlags().hasExact())
+      return isKnownNeverZero(Op.getOperand(0), Depth + 1);
 
+    KnownBits Op0 = computeKnownBits(Op.getOperand(0), Depth + 1);
+    KnownBits Op1 = computeKnownBits(Op.getOperand(1), Depth + 1);
+    if (Op0.isStrictlyPositive() && Op1.isStrictlyPositive()) {
+      std::optional<bool> uge = KnownBits::uge(Op0, Op1);
+      if (uge && *uge)
+        return true;
+    }
+
+    if (KnownBits::sdiv(Op0, Op1).isNonZero())
+      return true;
+    break;
+  }
   case ISD::ADD:
     if (Op->getFlags().hasNoUnsignedWrap())
       if (isKnownNeverZero(Op.getOperand(1), Depth + 1) ||
diff --git a/llvm/lib/Support/KnownBits.cpp b/llvm/lib/Support/KnownBits.cpp
index d72355dab6f1d3..fac9980a9349fc 100644
--- a/llvm/lib/Support/KnownBits.cpp
+++ b/llvm/lib/Support/KnownBits.cpp
@@ -979,7 +979,9 @@ KnownBits KnownBits::udiv(const KnownBits &LHS, const KnownBits &RHS,
   assert(!LHS.hasConflict() && !RHS.hasConflict());
   KnownBits Known(BitWidth);
 
-  if (LHS.isZero() || RHS.isZero()) {
+  // if LHS < RHS, then LHS / RHS is 0.
+  std::optional<bool> ult = KnownBits::ult(LHS, RHS);
+  if (LHS.isZero() || RHS.isZero() || (ult && *ult)) {
     // Result is either known Zero or UB. Return Zero either way.
     // Checking this earlier saves us a lot of special cases later on.
     Known.setAllZero();

Copy link

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

Copy link

✅ With the latest revision this PR passed the Python code formatter.

@topperc
Copy link
Collaborator

topperc commented Mar 25, 2024

Can you make the title more descriptive? Add more cases where?

@topperc
Copy link
Collaborator

topperc commented Mar 25, 2024

Tests?

if (uge && *uge)
return true;

if (KnownBits::udiv(Op0, Op1).isNonZero())
Copy link
Collaborator

@topperc topperc Mar 25, 2024

Choose a reason for hiding this comment

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

We fall back to calling computeKnownBits(Op, Depth).isNonZero() after the switch in this function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

just return KnownBits::udiv(Op0, Op1).isNonZero();

if (Op->getFlags().hasExact())
return isKnownNeverZero(Op.getOperand(0), Depth + 1);

// If Op1 <= Op0, then Op0 is at least 1, and therefore not 0.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// If Op1 <= Op0, then Op0 is at least 1, and therefore not 0.
// If Op0 >= Op1, then the result is at least 1, and therefore not 0.

// If Op1 <= Op0, then Op0 is at least 1, and therefore not 0.
KnownBits Op0 = computeKnownBits(Op.getOperand(0), Depth + 1);
KnownBits Op1 = computeKnownBits(Op.getOperand(1), Depth + 1);
std::optional<bool> uge = KnownBits::uge(Op0, Op1);
Copy link
Contributor

Choose a reason for hiding this comment

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

CamelCase for variables, here and elsewhere.

Comment on lines +982 to +984
// if LHS < RHS, then LHS / RHS is 0.
std::optional<bool> ult = KnownBits::ult(LHS, RHS);
if (LHS.isZero() || RHS.isZero() || (ult && *ult)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please split this into a separate patch with unit test coverage.

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

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

Please branch off the KnownBits.cpp changes into its own PR

Also, still waiting for test cases :)

std::optional<bool> uge = KnownBits::uge(Op0, Op1);
if (uge && *uge)
return true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

if (Op0.isStrictlyPositive() && Op1.isStrictlyPositive() &&
    KnownBits::uge(Op0, Op1).value_or(false))
  return true;

if (uge && *uge)
return true;

if (KnownBits::udiv(Op0, Op1).isNonZero())
Copy link
Collaborator

Choose a reason for hiding this comment

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

just return KnownBits::udiv(Op0, Op1).isNonZero();


if (KnownBits::sdiv(Op0, Op1).isNonZero())
return true;
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

return KnownBits::sdiv(Op0, Op1).isNonZero();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:SelectionDAG SelectionDAGISel as well llvm:support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants