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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Support/KnownBits.cpp:576
 
+  remGetLowBits(Known, LHS, RHS);
+
----------------
goldstein.w.n wrote:
> 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
>  
> How about "merge"? XD
Great idea! But here's what I came up with: D150443


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149421