[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 added inline comments.


================
Comment at: llvm/lib/Support/KnownBits.cpp:576
 
+  remGetLowBits(Known, LHS, RHS);
+
----------------
foad wrote:
> goldstein.w.n wrote:
> > 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.
> > I don't think this is right.
> Good point. You'd want some kind of "merge" operation which doesn't exist because I can't think of a good name for it.
> > I don't think this is right.
> Good point. You'd want some kind of "merge" operation which doesn't exist because I can't think of a good name for it.
How about "merge"? XD
 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149421