Probably these were not really bugs in this context, but I think using Op1 instead of Op2 makes more sense to people who read this code.<br><br><div class="gmail_quote">On Thu, Jan 17, 2013 at 7:47 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I created a new patch which factors out the code in SelectionDAGBuilder::visitShift and fixes the bug I mentioned in my previous email.<br>
<br>Note that, in the patch, I also fixed two lines in SelectionDAGBuilder::visitShift, which I think are bugs (please correct me if I am wrong). I believe these bugs haven't caused any problems in practice, but I think it is still important to fix them.<br>


<br>First, we should pass the type of LHS (the shiftee), not the type of the RHS (shift amount), to getShiftAmountTy:<br><br>-  MVT ShiftTy = TLI.getShiftAmountTy(Op2.getValueType());<br><br>Second, we should compute the log of the size of the LHS (the shiftee) when we decide whether ShiftSize is large enough:<br>


<br>-    else if (ShiftSize >= Log2_32_Ceil(Op2.getValueType().getSizeInBits()))<div class="HOEnZb"><div class="h5"><br><br><br><div class="gmail_quote">On Thu, Jan 17, 2013 at 2:38 PM, Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>></span> wrote:<br>


<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On Tue, Jan 15, 2013 at 2:51 PM, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>> wrote:<br>



><br>
> The attached patch is an attempt to fix the following bugs:<br>
><br>
> <a href="http://llvm.org/bugs/show_bug.cgi?id=14652" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=14652</a><br>
> <a href="http://llvm.org/bugs/show_bug.cgi?id=14740" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=14740</a><br>
><br>
> It fixes the code which scalarizes vector shifts and ensures that the type<br>
> of the shift amount operand is the type TargetLowering::getShiftAmountTy<br>
> returns. This is the same approach taken in SelectionDAGBuilder::visitShift.<br>
><br>
> The command that was failing in 14652 passes after this patch is applied<br>
> and fixes 14740 as well.<br>
<br>
</div></div>The approach seems fine.  Can you factor out the logic into a utility function?<br>
<span><font color="#888888"><br>
-Eli<br>
</font></span></blockquote></div><br>
</div></div></blockquote></div><br>