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

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev added inline comments.


================
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)
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > 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?
> (note that there's a typo in my diff, it should be
> `MinBitWidth = std::max(MinBitWidth, MinSignedBits);` not
> `MinBitWidth = std::max(MinBitWidth, NumSignBits);`)
Ok, thanks, used `CountMinSignBits()` for `ashr`.


================
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
----------------
lebedev.ri wrote:
> 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.
Ok, I've changed that lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108355