[llvm-commits] Fix for assertion in DAGCombiner

[Villmow, Micah]

Jan,
 One thing you can try is to use the helper function DAG.gteSExtOrTrunc() instead of return DAG.getNode(ISD::TRUNCATE, N->getDebugLoc(),  VT,  VsetCC); This should fix the problem of the case where the type is larger without duplicating code.

Micah

-----Original Message-----
From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Jan Sjodin
Sent: Monday, April 26, 2010 12:11 PM
To: Dan Gohman
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] Fix for assertion in DAGCombiner

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