[llvm-commits] PATCH: Fix bug in DAGCombiner generating illegal vector INT_TO_FP nodes

[Nadav Rotem]

Tom, 

This is unrelated to your fix, I don't agree with the motivation for "reduceBuildVecConvertToConvertBuildVec", and  think that it is the wrong way of solving the problem that the type-legalizer scalarizes conversion of illegal vector types.  

+
+  if (!TLI.isOperationLegalOrCustom(Opcode, NVT)) {
+    return SDValue();
+  }
+

No need for braces. 

LGTM. 


On Dec 30, 2012, at 6:55 AM, Tom Stellard <tom at stellard.net> wrote:

> Ping.
> 
> On Fri, Dec 21, 2012 at 08:25:38AM -0800, Tom Stellard wrote:
>> Hi,
>> 
>> The attached patch fixes a bug in
>> DAGCombiner::reduceBuildVecConvertToConvertBuildVec() where
>> the legality of vector INT_TO_FP and UINT_TO_FP operations was not being
>> checked correctly.
>> 
>> The test case X86/cvtv2f32.ll, which was added in the same commit that introduces
>> the bug now fails with this bug fix, so this patch marks it as xfail.
>> I took a quick look at this test, but I couldn't figure out how to fix it.
>> I did notice, however, that X86ISelLowering.cpp does this:
>> 
>> setOperationAction(ISD::UINT_TO_FP,       MVT::v2f32, Custom);
>> 
>> which is wrong. it should probably be v2i32 instead, but changing this
>> did not fix the X86/cvtv2f32.ll test.
>> 
>> -Tom
> 
>> diff --git lib/CodeGen/SelectionDAG/DAGCombiner.cpp lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> index c852c06..2ff5650 100644
>> --- lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> +++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> @@ -8624,11 +8624,8 @@ SDValue DAGCombiner::reduceBuildVecConvertToConvertBuildVec(SDNode *N) {
>>     if (Opcode == ISD::DELETED_NODE &&
>>         (Opc == ISD::UINT_TO_FP || Opc == ISD::SINT_TO_FP)) {
>>       Opcode = Opc;
>> -      // If not supported by target, bail out.
>> -      if (TLI.getOperationAction(Opcode, VT) != TargetLowering::Legal &&
>> -          TLI.getOperationAction(Opcode, VT) != TargetLowering::Custom)
>> -        return SDValue();
>>     }
>> +
>>     if (Opc != Opcode)
>>       return SDValue();
>> 
>> @@ -8653,6 +8650,11 @@ SDValue DAGCombiner::reduceBuildVecConvertToConvertBuildVec(SDNode *N) {
>>   assert(SrcVT != MVT::Other && "Cannot determine source type!");
>> 
>>   EVT NVT = EVT::getVectorVT(*DAG.getContext(), SrcVT, NumInScalars);
>> +
>> +  if (!TLI.isOperationLegalOrCustom(Opcode, NVT)) {
>> +    return SDValue();
>> +  }
>> +
>>   SmallVector<SDValue, 8> Opnds;
>>   for (unsigned i = 0; i != NumInScalars; ++i) {
>>     SDValue In = N->getOperand(i);
>> diff --git test/CodeGen/R600/dagcombiner-bug-illegal-vec4-int-to-fp.ll test/CodeGen/R600/dagcombiner-bug-illegal-vec4-int-to-fp.ll
>> new file mode 100644
>> index 0000000..1acf905
>> --- /dev/null
>> +++ test/CodeGen/R600/dagcombiner-bug-illegal-vec4-int-to-fp.ll
>> @@ -0,0 +1,33 @@
>> +;RUN: llc < %s -march=r600 -mcpu=redwood | FileCheck %s
>> +
>> +;CHECK: INT_TO_FLT T{{[0-9]+\.[XYZW], T[0-9]+\.[XYZW]}}
>> +
>> +; This test is for a bug in
>> +; DAGCombiner::reduceBuildVecConvertToConvertBuildVec() where
>> +; the wrong type was being passed to
>> +; TargetLowering::getOperationAction() when checking the legality of
>> +; ISD::UINT_TO_FP and ISD::SINT_TO_FP opcodes.
>> +
>> +define void @sint(<4 x float> addrspace(1)* %out, i32 addrspace(1)* %in) {
>> +entry:
>> +  %ptr = getelementptr i32 addrspace(1)* %in, i32 1
>> +  %sint = load i32 addrspace(1) * %in
>> +  %conv = sitofp i32 %sint to float
>> +  %0 = insertelement <4 x float> undef, float %conv, i32 0
>> +  %splat = shufflevector <4 x float> %0, <4 x float> undef, <4 x i32> zeroinitializer
>> +  store <4 x float> %splat, <4 x float> addrspace(1)* %out
>> +  ret void
>> +}
>> +
>> +;CHECK: UINT_TO_FLT T{{[0-9]+\.[XYZW], T[0-9]+\.[XYZW]}}
>> +
>> +define void @uint(<4 x float> addrspace(1)* %out, i32 addrspace(1)* %in) {
>> +entry:
>> +  %ptr = getelementptr i32 addrspace(1)* %in, i32 1
>> +  %uint = load i32 addrspace(1) * %in
>> +  %conv = uitofp i32 %uint to float
>> +  %0 = insertelement <4 x float> undef, float %conv, i32 0
>> +  %splat = shufflevector <4 x float> %0, <4 x float> undef, <4 x i32> zeroinitializer
>> +  store <4 x float> %splat, <4 x float> addrspace(1)* %out
>> +  ret void
>> +}
>> diff --git test/CodeGen/X86/cvtv2f32.ll test/CodeGen/X86/cvtv2f32.ll
>> index 466b096..d11bb9e 100644
>> --- test/CodeGen/X86/cvtv2f32.ll
>> +++ test/CodeGen/X86/cvtv2f32.ll
>> @@ -1,3 +1,7 @@
>> +; A bug fix in the DAGCombiner made this test fail, so marking as xfail
>> +; until this can be investigated further.
>> +; XFAIL: *
>> +
>> ; RUN: llc < %s -mtriple=i686-linux-pc -mcpu=corei7 | FileCheck %s
>> 
>> define <2 x float> @foo(i32 %x, i32 %y, <2 x float> %v) {
> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>