We shouldn't need to move this much code to fix this bug. Are you just trying to avoid having duplicate/similar code in the DAG combine and in the lowering path?<br><br><div class="gmail_quote">On Tue, Feb 19, 2013 at 10:33 AM, Michael Liao <span dir="ltr"><<a href="mailto:michael.liao@intel.com" target="_blank">michael.liao@intel.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Nadav<br>
<br>
The issue for me to split this patch is that it will break lots of<br>
regression tests. The new lowering is moved from the original shift<br>
combining plus a special handling in 32-bit mode.<br>
<br>
please check the latest patch following your other suggestion. The patch<br>
is revised to handle the canonicalized form of broadcast. Could you<br>
point me to more documents on the canonical forms of some useful<br>
patterns, such as broadcast.<br>
<br>
BTW, the current code style suggests 'funcName'. In that file, there're<br>
many existing functions still following the previous one but there're<br>
some following the new one. Do you need change to new one? I could take<br>
that if needed.<br>
<br>
Yours<br>
<span class="HOEnZb"><font color="#888888">- Michael<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Tue, 2013-02-19 at 09:42 -0800, Nadav Rotem wrote:<br>
> Hi Michael,<br>
><br>
> Thanks for working on this. In this patch you moved a lot of code around. Can you split it into two patches: refactoring and the new lowering ?<br>
><br>
> +static SDValue lowerScalarImmediateShift(SDValue Op, SelectionDAG &DAG,<br>
><br>
> Please match the coding style of the existing code. In this file most static functions start with an upper case (for example PromoteSplat, LowerBuildVector, etc).<br>
><br>
> + // Check remaiing shift amounts.<br>
> + for (unsigned i = Ratio; i != Amt.getNumOperands(); i += Ratio) {<br>
><br>
> Typo.<br>
><br>
> + %smear.0 = insertelement <8 x i32> undef, i32 %shiftval, i32 0<br>
> + %smear.1 = insertelement <8 x i32> %smear.0, i32 %shiftval, i32 1<br>
> + %smear.2 = insertelement <8 x i32> %smear.1, i32 %shiftval, i32 2<br>
> + %smear.3 = insertelement <8 x i32> %smear.2, i32 %shiftval, i32 3<br>
> + %smear.4 = insertelement <8 x i32> %smear.3, i32 %shiftval, i32 4<br>
> + %smear.5 = insertelement <8 x i32> %smear.4, i32 %shiftval, i32 5<br>
> + %smear.6 = insertelement <8 x i32> %smear.5, i32 %shiftval, i32 6<br>
> + %smear.7 = insertelement <8 x i32> %smear.6, i32 %shiftval, i32 7<br>
> + %bitop = lshr <8 x i32> %input, %smear.7<br>
> + ret <8 x i32> %bitop<br>
><br>
> The canonicalized form of broadcast is insert+shuffle.<br>
><br>
> Thanks,<br>
> Nadav<br>
><br>
> On Feb 19, 2013, at 12:32 AM, Michael Liao <<a href="mailto:michael.liao@intel.com">michael.liao@intel.com</a>> wrote:<br>
><br>
> > Hi All,<br>
> ><br>
> > This patch fixes PR15296 by lowering SRA/SRL/SHL into PSRA/PSRL/PSHL<br>
> > during DAG lowering instead of DAG combining, because 256-bit integer<br>
> > vector will be expanded in lowering where DAG combining will lose the<br>
> > chance to transform it.<br>
> ><br>
> > Thanks for your review<br>
> > - Michael<br>
> ><br>
> > <0001-Fix-PR15296.patch>_______________________________________________<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><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></blockquote></div><br><br clear="all"><div><br></div>-- <br>~Craig