[PATCH] D116500: [Support] Add KnownBits::countMaxSignedBits(). Make KnownBits::countMinSignBits() always return at least 1.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/Support/KnownBits.h:256-258
+  /// Returns the maximum number of bits needed to represent all possible
+  /// signed values with these known bits.
+  unsigned countMaxSignedBits() const {
----------------
spatel wrote:
> The name doesn't read clearly to me. Does this seem better?
>   /// Returns the maximum number of bits needed to represent all possible
>   /// signed values with these known bits. This is the inverse of the minimum 
>   /// number of known sign bits. Examples for bitwidth 5:
>   /// 110?? --> 4
>   /// 0000? --> 2 
>   unsigned countMaxSignificantBits() const {
> 
> I thought this was what countMaxActiveBits() returns, so we should put a comment on that too to avoid confusion.
> The "significant" terminology would be similar to a change in D20275.
countMaxActiveBits() treats it as an unsigned number. It treats each 1 in a negative number as significant and ignores all leading zeros on a postive number.

I agree the name countMaxSignedBits() isn't great. It's partially derived from APInt::getMinSignedBits(). Maybe we should start the rename there and lose the "Min"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116500