[PATCH] D108355: [AggressiveInstCombine] Add arithmetic shift right instr to `TruncInstCombine` DAG

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D108355#2956676 <https://reviews.llvm.org/D108355#2956676>, @anton-afanasyev wrote:

> Rebase after splitted and precommited tests



In D108355#2956680 <https://reviews.llvm.org/D108355#2956680>, @anton-afanasyev wrote:

> In D108355#2955549 <https://reviews.llvm.org/D108355#2955549>, @lebedev.ri wrote:
>
>> This patch does not apply for me.
>> Please precommit the tests, and rebase the patch so that it applies to `main`.
>
>
>
> In D108355#2955569 <https://reviews.llvm.org/D108355#2955569>, @lebedev.ri wrote:
>
>> Also, let's split `trunc_shifts.ll` into `trunc_{shl,lshr,ashr}.ll`.
>
> Ok, done

Thank you!



================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:299-307
+      if (I->getOpcode() == Instruction::AShr ||
+          I->getOpcode() == Instruction::LShr) {
         KnownBits KnownLHS = computeKnownBits(I->getOperand(0), DL);
         MinBitWidth =
-            std::max(MinBitWidth, KnownLHS.getMaxValue().getActiveBits());
+            std::max(MinBitWidth, OrigBitWidth - KnownLHS.countMinSignBits() +
+                                      KnownLHS.isNegative());
         if (MinBitWidth >= OrigBitWidth)
----------------
anton-afanasyev wrote:
> lebedev.ri wrote:
> > 
> I've intentionally merged this two cases, since `ashr` and `lshr` processing doesn't differ. What really matters is positivity/negativity of their operand. This is symmetrical cases (see updated summary).
> 
> For instance, for your change we still have to replace `ashr` with `lshr` if sign bits were zeros and with `ashr` if sign bits were ones. Why not to do the same for `lshr`?
Ehrm, these two inline comments are separate.
Can you explain why we should ask for known *bits* even for `ashr`,
even though we then ask for known *sign* bits?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:409-410
+      Value *RHS = getReducedOperand(I->getOperand(1), SclTy);
+      KnownBits KnownLHS = computeKnownBits(I->getOperand(0), DL);
+      Opc = KnownLHS.isNegative() ? Instruction::AShr : Instruction::LShr;
       // Preserve `exact` flag since truncation doesn't change exactness
----------------
anton-afanasyev wrote:
> lebedev.ri wrote:
> > [Why] do we have to do this?
> > Doesn't seem like something this transform should worry about?
> If sign bits were ones we have to replace original shift with `ashr`, `lshr` doesn't work (see `@lshr_negative_operand_but_short()` test).
I see. Please explain all that in a code comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108355