[PATCH] [DagCombiner] Generalized BuildVector Vector Concatenation

[Andrea Di Biagio]

In http://reviews.llvm.org/D7816#128498, @aditya_nandakumar wrote:

> Hi
>
> I have a question with this patch.
>
> Consider a DAG which looks like
>  v4i8 concat_vectors( v2i8 BUILD_VECTOR(i16 , i16),  v2i8 BUILD_VECTOR(i16, i16)). Consider that v2i8, v4i8 and i16 are legal types but i8 is not.
>
> Previously, we would have picked the minimum type from
>  EVT SclTy0 = N0.getOperand(0)->getValueType(0);   
>  EVT SclTy1 = N1.getOperand(0)->getValueType(0);
>  EVT MinTy = SclTy0.bitsLE(SclTy1) ? SclTy0 : SclTy1;
>  This would give us i16 as the MinType which is legal.
>
> After the patch, this would pick 
>  MinType =VT.getScalarType()
>  which would be i8 and this would be illegal on the target.
>
> I believe this happens after TypeLegalization. Is the transformation correct?


Hi Adytia,

I think you are right and the patch shouldn't have used 'getScalarType()'.
We cannot assume that 'VT.getScalarType()' always returns a legal type for the target.
Variable SVT should have been initialized to something like: 'N0.getOperand(0)->getValueType(0);', where N0 is the first operand to the concat_vector.

> Thanks

> Aditya





REPOSITORY
  rL LLVM

================
Comment at: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11450-11453
@@ -11453,8 +11449,6 @@
       // operand types. Get the smaller type and truncate all operands to it.
-      EVT MinTy = SclTy0.bitsLE(SclTy1) ? SclTy0 : SclTy1;
-      for (unsigned i = 0; i != BuildVecNumElts; ++i)
-        Opnds.push_back(DAG.getNode(ISD::TRUNCATE, SDLoc(N), MinTy,
-                        N0.getOperand(i)));
-      for (unsigned i = 0; i != BuildVecNumElts; ++i)
-        Opnds.push_back(DAG.getNode(ISD::TRUNCATE, SDLoc(N), MinTy,
-                        N1.getOperand(i)));
+      for (const SDValue &Op : N->ops()) {
+        EVT OpSVT = Op.getValueType().getScalarType();
+        MinVT = MinVT.bitsLE(OpSVT) ? MinVT : OpSVT;
+      }
+
----------------
We cannot assume that 'Op.getValueType().getScalarType()' and 'Op.getOperand(0)->getValueType(0)' are the same type.

I think we should replace 'Op.getValueType().getScalarType()' with 'Op.getOperand(0)->getValueType(0)'.

http://reviews.llvm.org/D7816

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/