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

[Jay Foad via Phabricator via llvm-commits]

foad 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:
> > 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()" ?
> Added to the other renames in D116522
> You know where I'm leaning. :)
> "getSignificantBits()" or "getNumSignificantBits()" ?

I already commented in D116522 but I really don't like that we ended up with APInt::getActiveBits vs APInt::getSignificantBits for the unsigned vs signed version of the same thing.

How about getNumBitsSigned (trying to avoid confusion with "sign bits") vs getNumBitsUnsigned?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116500