[PATCH] D150093: [KnownBits] Add implementation for `KnownBits::sdiv`

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Sorry I'm late.



================
Comment at: llvm/lib/Support/KnownBits.cpp:541
+                          bool Exact) {
+  // Equivilent of `udiv`. We must have caught this before it was folded.
+  if (LHS.isNonNegative() && RHS.isNonNegative())
----------------
Typo "Equivalent"


================
Comment at: llvm/lib/Support/KnownBits.cpp:549
+
+  APInt Num, Denum;
+  // Positive -> true
----------------
Typo "Denom"


================
Comment at: llvm/lib/Support/KnownBits.cpp:559
+    // Result non-negative.
+  } else if (LHS.isNegative() && RHS.isStrictlyPositive()) {
+    // Result is non-negative if Exact OR -LHS u>= RHS.
----------------
Why **strictly** positive here? I don't think you need to worry about divide-by-zero (poison) cases.


================
Comment at: llvm/lib/Support/KnownBits.cpp:560
+  } else if (LHS.isNegative() && RHS.isStrictlyPositive()) {
+    // Result is non-negative if Exact OR -LHS u>= RHS.
+    if (Exact || (-LHS.getSignedMaxValue()).uge(RHS.getSignedMaxValue())) {
----------------
Typoe "Result is negative ..."


================
Comment at: llvm/lib/Support/KnownBits.cpp:567
+  } else if (LHS.isStrictlyPositive() && RHS.isNegative()) {
+    // Result is non-negative if Exact OR LHS u>= -RHS.
+    if (Exact || LHS.getSignedMinValue().uge(-RHS.getSignedMinValue())) {
----------------
Typo "Result is negative ..."


================
Comment at: llvm/lib/Support/KnownBits.cpp:577
+    APInt Res = Num.sdiv(Denum);
+    if (*ResultSign) {
+      unsigned LeadZ = Res.countLeadingZeros();
----------------
At this point the sign of Res should match ResultSign, so you don't need a std::optional, you could just have a "bool ResultSignKnown".


================
Comment at: llvm/lib/Support/KnownBits.cpp:580
+      Known.Zero.setHighBits(LeadZ);
+      Known.makeNonNegative();
+    } else {
----------------
Don't need this, or the makeNegative call below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150093/new/

https://reviews.llvm.org/D150093