This is an archive of the discontinued LLVM Phabricator instance.

[DagCombiner] Generalized BuildVector Vector Concatenation
ClosedPublic

Authored by RKSimon on Feb 22 2015, 7:06 AM.

Details

Summary

The CONCAT_VECTORS combiner pass can transform the concat of two BUILD_VECTOR nodes into a single BUILD_VECTOR node.

This patch generalises this to support any number of BUILD_VECTOR nodes, and also permits UNDEF nodes to be included as well.

This was noticed as AVX vec128 -> vec256 canonicalization sometimes creates a CONCAT_VECTOR with a real vec128 lower and an vec128 UNDEF upper (see zext test modifications).

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 20476.Feb 22 2015, 7:06 AM
RKSimon retitled this revision from to [DagCombiner] Generalized BuildVector Vector Concatenation.
RKSimon updated this object.
RKSimon edited the test plan for this revision. (Show Details)
RKSimon added reviewers: qcolombet, chandlerc, andreadb.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: Unknown Object (MLST).
andreadb accepted this revision.Feb 22 2015, 9:05 AM
andreadb edited edge metadata.

Hi Simon,

The patch looks good to me.

Thanks!
Andrea

This revision is now accepted and ready to land.Feb 22 2015, 9:05 AM
This revision was automatically updated to reflect the committed changes.

Thanks Andrea

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?

Thanks
Aditya

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

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11450–11453 ↗(On Diff #20479)

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)'.

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.

Hi Guys - you're absolutely right - we need to use the build vector input operand value type, not the build vector scalar type. I will get a fix in shortly.