[llvm-commits] Fix for assertion in DAGCombiner

[Dan Gohman]

Hello,

I don't know if this is the right approach. Do you have an idea of
what kind of code you'd like to get out of a vector truncate?

Dan

On Apr 26, 2010, at 12:11 PM, Jan Sjodin wrote:

> Below is one way of fixing the issue of sign extending a vector of smaller element types.
> I suppose it would be fairly trivial to include the case where the type is larger by replacing
> truncate with a second sign extend, but I wanted to get feedback on this code:
> 
> Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/DAGCombiner.cpp    (revision 102340)
> +++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp    (working copy)
> @@ -3495,20 +3495,34 @@
> 
>   if (N0.getOpcode() == ISD::SETCC) {
>     // sext(setcc) -> sext_in_reg(vsetcc) for vectors.
> -    if (VT.isVector() &&
> +    // Only do this before legalize for now.
> +    if (VT.isVector() && !LegalOperations) {
> +      EVT N0VT = N0.getOperand(0).getValueType();
>         // We know that the # elements of the results is the same as the
>         // # elements of the compare (and the # elements of the compare result
>         // for that matter).  Check to see that they are the same size.  If so,
>         // we know that the element size of the sext'd result matches the
>         // element size of the compare operands.
> -        VT.getSizeInBits() == N0.getOperand(0).getValueType().getSizeInBits() &&
> -      
> -        // Only do this before legalize for now.
> -        !LegalOperations) {
> -      return DAG.getVSetCC(N->getDebugLoc(), VT, N0.getOperand(0),
> -                           N0.getOperand(1),
> -                           cast<CondCodeSDNode>(N0.getOperand(2))->get());
> -    }
> +      if(VT.getSizeInBits() == N0VT.getSizeInBits())
> +       return DAG.getVSetCC(N->getDebugLoc(), VT, N0.getOperand(0),
> +                            N0.getOperand(1),
> +                            cast<CondCodeSDNode>(N0.getOperand(2))->get());
> +      // If the desired elements are smaller than the source elements we can
> +      // use a matching integer vector type and then truncate
> +      else if(VT.getSizeInBits() < N0VT.getSizeInBits()) {
> +       EVT MatchingElementType =
> +         EVT::getIntegerVT(*DAG.getContext(),
> +                           N0VT.getScalarType().getSizeInBits());
> +       EVT MatchingVectorType =
> +         EVT::getVectorVT(*DAG.getContext(), MatchingElementType,
> +                          N0VT.getVectorNumElements());
> +       SDValue VsetCC = 
> +         DAG.getVSetCC(N->getDebugLoc(), MatchingVectorType, N0.getOperand(0),
> +                       N0.getOperand(1),
> +                       cast<CondCodeSDNode>(N0.getOperand(2))->get());
> +       return DAG.getNode(ISD::TRUNCATE, N->getDebugLoc(),  VT,  VsetCC);
> +      }
> +    }
> 
>     // sext(setcc x, y, cc) -> (select_cc x, y, -1, 0, cc)
>     unsigned ElementWidth = VT.getScalarType().getSizeInBits();
> 
> 
> 
> 
> ----- Original Message ----
>> From: Dan Gohman <gohman at apple.com>
>> To: Jan Sjodin <jan_sjodin at yahoo.com>
>> Cc: llvm-commits at cs.uiuc.edu
>> Sent: Fri, April 23, 2010 9:22:30 PM
>> Subject: Re: [llvm-commits] Fix for assertion in DAGCombiner
>> 
>> 
> On Apr 23, 2010, at 9:41 AM, Jan Sjodin wrote:
> 
>> Small fix that 
>> prevents an assertion in getConstant if VT is a vector type.
>> 
>> 
>> Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> 
>> ===================================================================
>> --- 
>> lib/CodeGen/SelectionDAG/DAGCombiner.cpp    (revision 102107)
>> 
>> +++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp    (working copy)
>> 
>> @@ -3453,7 +3453,8 @@
>> 
>>    // sext(setcc x, y, cc) 
>> -> (select_cc x, y, -1, 0, cc)
>>    SDValue NegOne =
>> 
>> -      
>> DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), VT);
>> +  
>> 
>> DAG.getConstant(APInt::getAllOnesValue(VT.getScalarType().getSizeInBits()),
>> 
>> +                    
>> VT);
>>    SDValue SCC =
>> 
>> SimplifySelectCC(N->getDebugLoc(), N0.getOperand(0), 
>> N0.getOperand(1),
>> 
>>        NegOne, DAG.getConstant(0, VT),
> 
> Thanks! This 
>> is now applied (with a trivial edit for 80 columns).
> 
>> 
>> I was 
>> going to use the example below, but there are still more errors to be fixed (see 
>> below):
>> 
>> ; ModuleID = 'minimal.bc'
>> target datalayout = 
>> "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:6\
>> 
>> 4:64-f80:128:128"
>> target triple = "x86_64-pc-linux-gnu"
>> 
>> 
>> define void @myfunction(<3 x double>* %foo, <3 x double>* %bar, 
>> <3 x i32>* %result) {
>> entry:
>> %foo.value = load <3 
>> x double>* %foo            ; <<3 x 
>> double>> [#uses=1]
>> %bar.value = load <3 x double>* 
>> %bar            ; <<3 x double>> 
>> [#uses=1]
>> %cmp.i.i = fcmp oeq <3 x double> %foo.value, 
>> %bar.value ; <<3 x i1>> [#uses=1]
>> %cmp.ext.i.i = sext 
>> <3 x i1> %cmp.i.i to <3 x i32> ; <<3 x i32>> 
>> [#uses=1]
>> store <3 x i32> %cmp.ext.i.i, <3 x i32>* 
>> %result
>> ret void
>> }
>> 
>> 
>> llc 
>> fails:
>> WidenVectorOperand op #0: 0xb7fc520: i8 = setcc 0xb7fbe38, 
>> 0xb7fbec0, 0xb7fbf48
>> [ID=0]
>> Do not know how to widen this 
>> operator's operand!
>> UNREACHABLE executed at 
>> LegalizeVectorTypes.cpp:1898!
>> 
>> I will try and look into this as 
>> well.
> 
> On x86, support for vector SETCC is currently limited to cases 
>> where the
> result is sign-extended to a vector of the same width as the SETCC 
>> operand
> vector. In other words, if you change the <3 x i32> to <3 x 
>> i64> here 
> it'll work, because i64 is the same width as 
>> double.
> 
> Dan