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

[Ayman Musa via Phabricator via llvm-commits]

aymanmus marked an inline comment as done.
aymanmus added a comment.

In D74484#1873830 <https://reviews.llvm.org/D74484#1873830>, @nikic wrote:

> @aymanmus I have something along these lines in mind...
>
>   %conv = sext i8 %a to i32
>   %conv2 = sext i8 %b to i32
>   %conv3 = sext i8 %c to i32
>   %cmp = icmp slt i32 %conv3, TOO_BIG_CONST
>   %cond = select i1 %cmp, i32 %conv2, i32 %conv
>   %conv4 = trunc i32 %cond to i16
>   ret i16 %conv4
>
>
> That is, where the `icmp` has a form that passes the `isConstOrExt(C->getOperand(0)) && isConstOrExt(C->getOperand(1))` check, but later gets rejected due to constant bitwidth.
>
> Not sure if that example does it, but I think there must be some case like this, unless I'm misunderstanding the patch.


Totally agree.
Fixed the code to exclude the ICmp instruction from the part where we make the decision whether to transform or not.
ICmp instruction can only be replaced after we're already applying the transformation, and only if its operands can fit in the type we are converting to.



================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:180
+  return IsSigned ? MinBits + 1 : MinBits;
+}
+
----------------
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.


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

https://reviews.llvm.org/D74484