[PATCH] D58650: Add support for computing "zext of value" in KnownBits. NFCI
Simon Pilgrim via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 28 13:56:15 PST 2019
RKSimon added inline comments.
================
Comment at: llvm/trunk/include/llvm/Support/KnownBits.h:120
+ /// With ExtendedBitsAreKnownZero=false the extended bits are set to unknown.
+ KnownBits zext(unsigned BitWidth, bool ExtendedBitsAreKnownZero) {
+ unsigned OldBitWidth = getBitWidth();
----------------
bjope wrote:
> nikic wrote:
> > Might be clearer to separate this into a `zext` and an `anyext` method?
> Simply changing the semantics of zext in a single step seemed a little bit risky (at least considering OOT targets).
> We might wanna do that split in the future, assuming that OOT targets have had time to adapt to this two-argument version first. That is also why I decided not to use a default value for `ExtendedBitsAreKnownZero`.
>
> One thing that I'm not sure of is if these methods that map directly to the APInt names are supposed to indicate that we apply the APInt function on `One` and `Zero` (i.e. the name should map to the operation applied on the underlying bitmaps)? We already have `computeForAddSub` that is named based on the operations applied to the tracked value. Maybe we should add `computeForZext `and `computeForSext` instead, to make the naming more consistent for methods that refer to the operation on the tracked value.
>
>
I agree with @bjope - zext is pretty ambiguous, so including the explicit bool flag makes sense to me. 'zextKnownZeroBits()' and 'zextUnknownBits()' style methods would be alternatives but aren't any better afaict.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58650/new/
https://reviews.llvm.org/D58650
More information about the llvm-commits
mailing list