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

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev marked an inline comment as done.
anton-afanasyev added inline comments.


================
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:
> > > 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.
> Actually, let me backtrack this.
> This patch adds support for `ashr` truncation.
> It shouldn't also suddenly subtly change reasoning for `lshr`.
> Can we please not do that?
> Can we please not do that?
Sure, I've eventually done it that way: `lshr` is transformed as before, `ashr` transformation works for numbers treated as signed ones.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108355