[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 15:37, Benjamin Kramer wrote:
>
> On 27.01.2013, at 15:25, Duncan Sands <baldrick at free.fr> wrote:
>
>> 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.
>
> It first checks if the shl_parts operation (and the type it's shifting) is legal, but if it's using an illegal type for the shift amount it's obviously violating that contract. The legalizer then falls over because it assumes that illegal shl_parts do not occur. I guess we could assert that the shift amount always fits in the amount type if that would reduce your concerns.

I see, it should be OK then.

Ciao, Duncan.

>
> - Ben
>
>>
>> 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
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>