[PATCH] D58650: Add support for computing "zext of value" in KnownBits. NFCI

[Simon Pilgrim via Phabricator via llvm-commits]

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