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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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 {
----------------
craig.topper wrote:
> 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"?
Yes, there's a lot of mismatched naming. There's similar functionality in ConstantRange too. 
Starting with APInt sounds good. So we have getNumSignBits() there, and then getMinSignedBits() is derived from that. 
If we rename the latter to getSignedBits(), that could easily be confused with getNumSignBits().
You know where I'm leaning. :)
"getSignificantBits()" or "getNumSignificantBits()" ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116500