[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:
> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116500