[PATCH] D150923: [KnownBits] Factor out and improve the lowbit computation for {u,s}div

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked 2 inline comments as done.
goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Support/KnownBits.cpp:551-562
+  // Odd / X -> Odd
+  //    X must be odd, otherwise the div is not exact.
+  // EvenX / EvenY [if TZ(X) == TZ(Y)] -> Odd
+  if (LHS.One[LHSMinTZ] &&
+      (LHSMinTZ == 0 ||
+       (RHS.countMinTrailingZeros() == LHSMinTZ && RHS.One[LHSMinTZ])))
+    KnownOut.One.setBit(0);
----------------
foad wrote:
> Can I suggest a slightly more unified way of handling both of these cases?
> ```
>   int MinTZ = (int)LHS.countMinTrailingZeros() - (int)RHS.countMaxTrailingZeros();
>   int MaxTZ = (int)LHS.countMaxTrailingZeros() - (int)RHS.countMinTrailingZeros();
>   if (MinTZ >= 0) {
>     // Result has at least MinTZ trailing zeros.
>     KnownOut.Zero.setLowBits(MinTZ);
>     if (MinTZ == MaxTZ) { // might also need to check this is < BitWidth ???
>       // Result has exactly MinTZ trailing zeros.
>       KnownOut.One.setBit(MinTZ);
>     }
>   }
> ```
> Can I suggest a slightly more unified way of handling both of these cases?
> ```
>   int MinTZ = (int)LHS.countMinTrailingZeros() - (int)RHS.countMaxTrailingZeros();
>   int MaxTZ = (int)LHS.countMaxTrailingZeros() - (int)RHS.countMinTrailingZeros();
>   if (MinTZ >= 0) {
>     // Result has at least MinTZ trailing zeros.
>     KnownOut.Zero.setLowBits(MinTZ);
>     if (MinTZ == MaxTZ) { // might also need to check this is < BitWidth ???
>       // Result has exactly MinTZ trailing zeros.
>       KnownOut.One.setBit(MinTZ);
>     }
>   }
> ```

Nice! Just need to add:
```
  if (LHS.One[0])
    Known.One.setBit(0);
```

Because the rest of the logic relies on knowing LHS and RHS whereas LHS Odd alone is enough to imply the lowbit.


================
Comment at: llvm/lib/Support/KnownBits.cpp:635
 
-  if (Exact) {
-    // Odd / Odd -> Odd
-    if (LHS.One[0] && RHS.One[0]) {
-      Known.Zero.clearBit(0);
-      Known.One.setBit(0);
-    }
-    // Even / Odd -> Even
-    else if (LHS.Zero[0] && RHS.One[0]) {
-      Known.One.clearBit(0);
-      Known.Zero.setBit(0);
-    }
-    // Odd / Even -> impossible
-    // Even / Even -> unknown
-  }
+  Known = divComputeLowBit(Known, LHS, RHS, Exact);
 
----------------
foad wrote:
> Nit: could use intersectWith instead of passing in Known.
Not `unionWith`?
We need to check `conflict` as the tests test alot of poison values. I thought it would be cleaner (and less bugprone) to isolate that check to where its needed rather than put it in the common path which would be required if we made `unionWith`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150923