[PATCH] D74484: [AggressiveInstCombine] Add support for ICmp instr that feeds a select intsr's condition operand.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:180
+  return IsSigned ? MinBits + 1 : MinBits;
+}
+
----------------
aymanmus wrote:
> lebedev.ri wrote:
> > Correct me if i'm wrong, but won't this just work?
> > ```
> > static unsigned getConstMinBitWidth(bool IsSigned, ConstantInt *C) {
> >   const APInt& Val = C->getValue();
> >   unsigned NonSignBits = Val.getBitWidth() - Val.getNumSignBits();
> > 
> >   return IsSigned ? 1 + NonSignBits : NonSignBits;
> > }
> > ```
> > 
> IsSigned indicates whether the use of the constant is signed or not.
> I guess the "sign bit" of the APInt is deduced simply according to the content of the MSB.
> But think of the case where IsSigned = false, and APInt is of width 16 and value = 0xff80.
> Val.getNumSignBits() would return 9 (as if the value is signed and had 9 sign bits) and the function's return value would be 7.
> While instead, we expect to get 16 as a return value.
> Added a test case (@cmp_select_unsigned_const_i16_MSB1) that unjustifiably applies the transformation with the suggested code above and refrain from that with the current code.
> 
> However, the previous comments are 100% legit.
Right. What about this then:
```
static unsigned getConstMinBitWidth(bool IsSigned, ConstantInt *C) {
  const APInt& Val = C->getValue();
  return IsSigned ? Val.getMinSignedBits() : Val.getActiveBits();
}
```
?



================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:178
+  // Otherwise, count leading zeroes.
+  auto MinBits = Val.getActiveBits();
+  return IsSigned ? MinBits + 1 : MinBits;
----------------
Please explicitly spell out `unsigned` here


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

https://reviews.llvm.org/D74484