<div dir="ltr"><div><div>Hi Hal,<br><br></div>Thanks for pointing out. I somehow missed that. rectified in r213295.<br><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jul 17, 2014 at 7:37 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">----- Original Message -----<br>
> From: "Suyog Sarda" <<a href="mailto:suyog.sarda@samsung.com">suyog.sarda@samsung.com</a>><br>
> To: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> Sent: Thursday, July 17, 2014 1:28:15 AM<br>
> Subject: [llvm] r213231 - Move ashr optimization from InstCombineShift to     InstSimplify.<br>
><br>
> Author: suyog<br>
> Date: Thu Jul 17 01:28:15 2014<br>
> New Revision: 213231<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=213231&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=213231&view=rev</a><br>
> Log:<br>
> Move ashr optimization from InstCombineShift to InstSimplify.<br>
> Refactor code, no functionality change, test case moved from<br>
> instcombine to instsimplify.<br>
><br>
> Differential Revision: <a href="http://reviews.llvm.org/D4102" target="_blank">http://reviews.llvm.org/D4102</a><br>
><br>
><br>
> Added:<br>
>     llvm/trunk/test/Transforms/InstSimplify/ashr-nop.ll<br>
> Removed:<br>
>     llvm/trunk/test/Transforms/InstCombine/ashr-nop.ll<br>
> Modified:<br>
>     llvm/trunk/lib/Analysis/InstructionSimplify.cpp<br>
>     llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp<br>
><br>
> Modified: llvm/trunk/lib/Analysis/InstructionSimplify.cpp<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InstructionSimplify.cpp?rev=213231&r1=213230&r2=213231&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InstructionSimplify.cpp?rev=213231&r1=213230&r2=213231&view=diff</a><br>

> ==============================================================================<br>
> --- llvm/trunk/lib/Analysis/InstructionSimplify.cpp (original)<br>
> +++ llvm/trunk/lib/Analysis/InstructionSimplify.cpp Thu Jul 17<br>
> 01:28:15 2014<br>
> @@ -1346,6 +1346,11 @@ static Value *SimplifyAShrInst(Value *Op<br>
>        cast<OverflowingBinaryOperator>(Op0)->hasNoSignedWrap())<br>
>      return X;<br>
><br>
> +  // Arithmetic shifting an all-sign-bit value is a no-op.<br>
> +  unsigned NumSignBits = ComputeNumSignBits(Op0);<br>
<br>
</div></div>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:<br>

<br>
  ComputeNumSignBits(Op0, Q.DL);<br>
<br>
Please fix this.<br>
<br>
Thanks,<br>
Hal<br>
<div class="HOEnZb"><div class="h5"><br>
> +  if (NumSignBits == Op0->getType()->getScalarSizeInBits())<br>
> +    return Op0;<br>
> +<br>
>    return nullptr;<br>
>  }<br>
><br>
><br>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp?rev=213231&r1=213230&r2=213231&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp?rev=213231&r1=213230&r2=213231&view=diff</a><br>

> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp<br>
> (original)<br>
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp Thu<br>
> Jul 17 01:28:15 2014<br>
> @@ -815,10 +815,5 @@ Instruction *InstCombiner::visitAShr(Bin<br>
>                          APInt::getSignBit(I.getType()->getScalarSizeInBits())))<br>
>      return BinaryOperator::CreateLShr(Op0, Op1);<br>
><br>
> -  // Arithmetic shifting an all-sign-bit value is a no-op.<br>
> -  unsigned NumSignBits = ComputeNumSignBits(Op0);<br>
> -  if (NumSignBits == Op0->getType()->getScalarSizeInBits())<br>
> -    return ReplaceInstUsesWith(I, Op0);<br>
> -<br>
>    return nullptr;<br>
>  }<br>
><br>
> Removed: llvm/trunk/test/Transforms/InstCombine/ashr-nop.ll<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/ashr-nop.ll?rev=213230&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/ashr-nop.ll?rev=213230&view=auto</a><br>

> ==============================================================================<br>
> --- llvm/trunk/test/Transforms/InstCombine/ashr-nop.ll (original)<br>
> +++ llvm/trunk/test/Transforms/InstCombine/ashr-nop.ll (removed)<br>
> @@ -1,8 +0,0 @@<br>
> -; RUN: opt < %s -instcombine -S | not grep ashr<br>
> -<br>
> -define i32 @foo(i32 %x) {<br>
> -  %o = and i32 %x, 1<br>
> -  %n = add i32 %o, -1<br>
> -  %t = ashr i32 %n, 17<br>
> -  ret i32 %t<br>
> -}<br>
><br>
> Added: llvm/trunk/test/Transforms/InstSimplify/ashr-nop.ll<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstSimplify/ashr-nop.ll?rev=213231&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstSimplify/ashr-nop.ll?rev=213231&view=auto</a><br>

> ==============================================================================<br>
> --- llvm/trunk/test/Transforms/InstSimplify/ashr-nop.ll (added)<br>
> +++ llvm/trunk/test/Transforms/InstSimplify/ashr-nop.ll Thu Jul 17<br>
> 01:28:15 2014<br>
> @@ -0,0 +1,10 @@<br>
> +; RUN: opt < %s -instsimplify -S | FileCheck %s<br>
> +<br>
> +; CHECK-LABEL: @foo<br>
> +; CHECK-NOT: ashr<br>
> +define i32 @foo(i32 %x) {<br>
> + %o = and i32 %x, 1<br>
> + %n = add i32 %o, -1<br>
> + %t = ashr i32 %n, 17<br>
> + ret i32 %t<br>
> +}<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span><div class="HOEnZb"><div class="h5">_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br>With regards,<br>Suyog Sarda<br>
</div>