[llvm-commits] [llvm] r173615 - When the legalizer is splitting vector shifts, the result may not have the right shift amount type.

[Duncan Sands]

Hi Benjamin,

On 27/01/13 12:19, Benjamin Kramer wrote:
> Author: d0k
> Date: Sun Jan 27 05:19:11 2013
> New Revision: 173615
>
> URL: http://llvm.org/viewvc/llvm-project?rev=173615&view=rev
> Log:
> When the legalizer is splitting vector shifts, the result may not have the right shift amount type.
>
> Fix that by adding a cast to the shift expander. This came up with vector shifts
> on sse-less X86 CPUs.
>
>     <2 x i64>       = shl <2 x i64> <2 x i64>
> -> i64,i64         = shl i64 i64; shl i64 i64
> -> i32,i32,i32,i32 = shl_parts i32 i32 i64; shl_parts i32 i32 i64
>
> Now we cast the last two i64s to the right type. Fixes the crash in PR14668.

I didn't really get it.  If shl_parts requires the Amt type to match the value
type, why are you using getShiftAmountTy for it and not the type of the other
operands?  If it isn't required to match the others, then why is i64 wrong?
Remember that this is type legalization, not operation legalization, and there
is the danger that getShiftAmountTy isn't big enough if the type of the shifted
quantity is huge.  For example, for scalars on x86, getShiftAmountTy returns
i8.  If you have a shift like:
   shl i4096 %x, i4096 999
then replacing the i4096 shift type with an i8 (and thus truncating the 999) is
just wrong.  Don't you have the same issue here?  This is why it is not safe to
use getShiftAmountTy in the type legalization stuff in general, and fixing up
the shift amount type to i8 or whatever has to be done later.

Ciao, Duncan.

>
> Modified:
>      llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
>      llvm/trunk/test/CodeGen/X86/legalize-shift-64.ll
>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp?rev=173615&r1=173614&r2=173615&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp Sun Jan 27 05:19:11 2013
> @@ -2095,9 +2095,16 @@ void DAGTypeLegalizer::ExpandIntRes_Shif
>       // Expand the subcomponents.
>       SDValue LHSL, LHSH;
>       GetExpandedInteger(N->getOperand(0), LHSL, LHSH);
> -
> -    SDValue Ops[] = { LHSL, LHSH, N->getOperand(1) };
>       EVT VT = LHSL.getValueType();
> +
> +    // If the shift amount operand is coming from a vector legalization it may
> +    // not have the right return type.  Fix that first by casting the operand.
> +    SDValue ShiftOp = N->getOperand(1);
> +    MVT ShiftTy = TLI.getShiftAmountTy(VT);
> +    if (ShiftOp.getValueType() != ShiftTy)
> +      ShiftOp = DAG.getZExtOrTrunc(ShiftOp, dl, ShiftTy);
> +
> +    SDValue Ops[] = { LHSL, LHSH, ShiftOp };
>       Lo = DAG.getNode(PartsOpc, dl, DAG.getVTList(VT, VT), Ops, 3);
>       Hi = Lo.getValue(1);
>       return;
>
> Modified: llvm/trunk/test/CodeGen/X86/legalize-shift-64.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/legalize-shift-64.ll?rev=173615&r1=173614&r2=173615&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/legalize-shift-64.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/legalize-shift-64.ll Sun Jan 27 05:19:11 2013
> @@ -54,3 +54,14 @@ define i64 @test4(i64 %xx, i32 %test) no
>   ; CHECK: orl	%esi, %eax
>   ; CHECK: sarl	%cl, %edx
>   }
> +
> +; PR14668
> +define <2 x i64> @test5(<2 x i64> %A, <2 x i64> %B) {
> +  %shl = shl <2 x i64> %A, %B
> +  ret <2 x i64> %shl
> +; CHECK: test5
> +; CHECK: shl
> +; CHECK: shldl
> +; CHECK: shl
> +; CHECK: shldl
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>