[PATCH] D108201: [AggressiveInstCombine] Add logical shift right instr to `TruncInstCombine` DAG

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev marked 3 inline comments as done.
anton-afanasyev added a comment.

In D108201#2952032 <https://reviews.llvm.org/D108201#2952032>, @spatel wrote:

> In D108201#2951907 <https://reviews.llvm.org/D108201#2951907>, @anton-afanasyev wrote:
>
>> Rebase after `shl` bugfix
>
> Add tests similar to 0988488ed461 <https://reviews.llvm.org/rG0988488ed461f5a862f23f9840e0750bef07f11c> (multiple widths) and 803270c0c691 <https://reviews.llvm.org/rG803270c0c691ad0dbf8ac91492c79aad9e0d023d> (64-bit) for lshr patterns?

Sure, added tests



================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:295-296
+        KnownBits KnownLHS = computeKnownBits(I->getOperand(0), DL);
+        MinBitWidth = std::max(MinBitWidth,
+                               SrcBitWidth - KnownLHS.countMinLeadingZeros());
+      }
----------------
spatel wrote:
> This wasn't updated with the suggested change to use getActiveBits().
> IIUC, we can make this more efficient (avoid computeKnownBits in some cases) by hoisting the common check for MinBitWidth >= OrigBitWidth above the extra check for lshr:
>       unsigned MinBitWidth = KnownRHS.getMaxValue()
>                                  .uadd_sat(APInt(SrcBitWidth, 1))
>                                  .getLimitedValue(SrcBitWidth);
>       if (MinBitWidth >= OrigBitWidth)
>         return nullptr;
>       if (I->getOpcode() == Instruction::LShr) {
>         KnownBits KnownLHS = computeKnownBits(I->getOperand(0), DL);
>         if (KnownLHS.getMaxValue().getActiveBits() >= OrigBitWidth)
>           return nullptr;
>       }
>       Itr.second.MinBitWidth = MinBitWidth;
> 
Thanks, duplicated check for efficiency.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:295
+        MinBitWidth = std::max(MinBitWidth,
+                               SrcBitWidth - KnownLHS.countMinLeadingZeros());
+      }
----------------
lebedev.ri wrote:
> 
Thanks, used `getActiveBits()`


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/trunc_shifts.ll:163
   %zext = zext i16 %x to i32
   %lshr = lshr i32 %zext, 16
   %trunc = trunc i32 %lshr to i16
----------------
spatel wrote:
> Similar to the previous `shl` example - this is another simplification to 0 that isn't handled by -instsimplify (but regular -instcombine gets it).
Ok, but here it is simple negative test, checking we do not fold to poisonous `lshr i16 x, 16`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108201