[PATCH] D149421: [KnownBits] Improve `KnownBits::rem(X, Y)` in cases where we can deduce low-bits of output

[Noah Goldstein via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Support/KnownBits.cpp:576
 
+  remGetLowBits(Known, LHS, RHS);
+
----------------
foad wrote:
> Just a matter of taste, but I'd prefer to write this as `Known |= remGetLowBits(LHS, RHS);`.
> 
> Also could move this above the `RHS.isConstant` case - then that case would only need to handle setting the high bits of Known.Zero.
> Just a matter of taste, but I'd prefer to write this as `Known |= remGetLowBits(LHS, RHS);`.
> 
I don't think this is right. `KnowBits::|` does `Known.Zero &= remGetLowBits.Zero`. We want the opposite. If some of the lowbits of `X` are zero in this case, we want to forcefully set those bits in `Known`.

> Also could move this above the `RHS.isConstant` case - then that case would only need to handle setting the high bits of Known.Zero.

sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149421