[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
llvm-commits at lists.llvm.org
Fri May 12 05:47:41 PDT 2023
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
More information about the llvm-commits
mailing list