[llvm] r213231 - Move ashr optimization from InstCombineShift to InstSimplify.

[suyog sarda]

Hi Hal,

Thanks for pointing out. I somehow missed that. rectified in r213295.



On Thu, Jul 17, 2014 at 7:37 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> ----- Original Message -----
> > From: "Suyog Sarda" <suyog.sarda at samsung.com>
> > To: llvm-commits at cs.uiuc.edu
> > Sent: Thursday, July 17, 2014 1:28:15 AM
> > Subject: [llvm] r213231 - Move ashr optimization from InstCombineShift
> to     InstSimplify.
> >
> > Author: suyog
> > Date: Thu Jul 17 01:28:15 2014
> > New Revision: 213231
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=213231&view=rev
> > Log:
> > Move ashr optimization from InstCombineShift to InstSimplify.
> > Refactor code, no functionality change, test case moved from
> > instcombine to instsimplify.
> >
> > Differential Revision: http://reviews.llvm.org/D4102
> >
> >
> > Added:
> >     llvm/trunk/test/Transforms/InstSimplify/ashr-nop.ll
> > Removed:
> >     llvm/trunk/test/Transforms/InstCombine/ashr-nop.ll
> > Modified:
> >     llvm/trunk/lib/Analysis/InstructionSimplify.cpp
> >     llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp
> >
> > Modified: llvm/trunk/lib/Analysis/InstructionSimplify.cpp
> > URL:
> >
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InstructionSimplify.cpp?rev=213231&r1=213230&r2=213231&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Analysis/InstructionSimplify.cpp (original)
> > +++ llvm/trunk/lib/Analysis/InstructionSimplify.cpp Thu Jul 17
> > 01:28:15 2014
> > @@ -1346,6 +1346,11 @@ static Value *SimplifyAShrInst(Value *Op
> >        cast<OverflowingBinaryOperator>(Op0)->hasNoSignedWrap())
> >      return X;
> >
> > +  // Arithmetic shifting an all-sign-bit value is a no-op.
> > +  unsigned NumSignBits = ComputeNumSignBits(Op0);
>
> This is not quite right. When this code was in InstCombine, you were
> calling the version of ComputeNumSignBits in InstCombine.h that
> automatically added the DataLayout* before calling into ValueTracking.
> Here, you're calling into ValueTracking directly without passing in the
> DataLayout*. I believe this should be:
>
>   ComputeNumSignBits(Op0, Q.DL);
>
> Please fix this.
>
> Thanks,
> Hal
>
> > +  if (NumSignBits == Op0->getType()->getScalarSizeInBits())
> > +    return Op0;
> > +
> >    return nullptr;
> >  }
> >
> >
> > Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp
> > URL:
> >
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp?rev=213231&r1=213230&r2=213231&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp
> > (original)
> > +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp Thu
> > Jul 17 01:28:15 2014
> > @@ -815,10 +815,5 @@ Instruction *InstCombiner::visitAShr(Bin
> >
>  APInt::getSignBit(I.getType()->getScalarSizeInBits())))
> >      return BinaryOperator::CreateLShr(Op0, Op1);
> >
> > -  // Arithmetic shifting an all-sign-bit value is a no-op.
> > -  unsigned NumSignBits = ComputeNumSignBits(Op0);
> > -  if (NumSignBits == Op0->getType()->getScalarSizeInBits())
> > -    return ReplaceInstUsesWith(I, Op0);
> > -
> >    return nullptr;
> >  }
> >
> > Removed: llvm/trunk/test/Transforms/InstCombine/ashr-nop.ll
> > URL:
> >
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/ashr-nop.ll?rev=213230&view=auto
> >
> ==============================================================================
> > --- llvm/trunk/test/Transforms/InstCombine/ashr-nop.ll (original)
> > +++ llvm/trunk/test/Transforms/InstCombine/ashr-nop.ll (removed)
> > @@ -1,8 +0,0 @@
> > -; RUN: opt < %s -instcombine -S | not grep ashr
> > -
> > -define i32 @foo(i32 %x) {
> > -  %o = and i32 %x, 1
> > -  %n = add i32 %o, -1
> > -  %t = ashr i32 %n, 17
> > -  ret i32 %t
> > -}
> >
> > Added: llvm/trunk/test/Transforms/InstSimplify/ashr-nop.ll
> > URL:
> >
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstSimplify/ashr-nop.ll?rev=213231&view=auto
> >
> ==============================================================================
> > --- llvm/trunk/test/Transforms/InstSimplify/ashr-nop.ll (added)
> > +++ llvm/trunk/test/Transforms/InstSimplify/ashr-nop.ll Thu Jul 17
> > 01:28:15 2014
> > @@ -0,0 +1,10 @@
> > +; RUN: opt < %s -instsimplify -S | FileCheck %s
> > +
> > +; CHECK-LABEL: @foo
> > +; CHECK-NOT: ashr
> > +define i32 @foo(i32 %x) {
> > + %o = and i32 %x, 1
> > + %n = add i32 %o, -1
> > + %t = ashr i32 %n, 17
> > + ret i32 %t
> > +}
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>



-- 
With regards,
Suyog Sarda
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140718/a41eb6dc/attachment.html>