[llvm-commits] [llvm] r59399 - in /llvm/trunk: lib/CodeGen/SelectionDAG/SelectionDAGBuild.cpp test/CodeGen/X86/vec_shuffle-25.ll test/CodeGen/X86/vec_shuffle-26.ll test/CodeGen/X86/vec_shuffle-27.ll

[Mon Ping Wang]

Hi Evan,

On Nov 18, 2008, at 10:00 AM, Evan Cheng wrote:

> Hi Mon Ping,
>
> Thanks. Some nitpicks below.
>
> Evan
>
> On Nov 15, 2008, at 9:06 PM, Mon P Wang wrote:
>
>> [deleted text]
>>
>>
>> void SelectionDAGLowering::visitShuffleVector(User &I) {
>> -  SDValue V1   = getValue(I.getOperand(0));
>> -  SDValue V2   = getValue(I.getOperand(1));
>> +  SDValue Srcs[2];
>> +  Srcs[0] = getValue(I.getOperand(0));
>> +  Srcs[1] = getValue(I.getOperand(1));
>
> A common idiom used is:
> SDValue Srcs[] = { getValue(I.getOperand(0),
> getValue(I.getOperand(1)) };
>
> Is an array preferrable to V1 and V2?
>

Probably not.  Src1 and Src2 is preferable to V1 and V2 as it is a  
little more descriptive.  I'll make the change.

>
>>
>>  SDValue Mask = getValue(I.getOperand(2));
>>
>>  MVT VT = TLI.getValueType(I.getType());
>> -  MVT VT1 = V1.getValueType();
>> -  unsigned MaskNumElts = Mask.getNumOperands();
>> -  unsigned Src1NumElts = VT1.getVectorNumElements();
>> +  MVT SrcVT = Srcs[0].getValueType();
>> +  int MaskNumElts = Mask.getNumOperands();
>> +  int SrcNumElts = SrcVT.getVectorNumElements();
>
> Why int instead of unsigned?
>

When I was checking the range for the indexes, I set the values to be  
out of range, i.e., -1 and SrcNumElts.  This makes the range a integer  
and since I compare with SrcNumElts, I would be doing a signed to  
unsigned comparison.  Making them all int avoid this.

>> [Deleted Text]
>> +
>> +  if (SrcNumElts < MaskNumElts && MaskNumElts % SrcNumElts == 0) {
>> +    // Mask is longer than the source vectors and is a multiple of
>> the source
>> +    // vectors.  We can use concatenate vector to make the mask and
>> vectors
>> +    // length match.
>
> lengthes.
>

I'll fix this.

>> +    if (SrcNumElts*2 == MaskNumElts && SequentialMask(Mask, 0)) {
>> +      // The shuffle is concatenating two vectors together.
>> +      setValue(&I, DAG.getNode(ISD::CONCAT_VECTORS, VT, Srcs[0],
>> Srcs[1]));
>>      return;
>>    }
>>
>> -    // Pad both vectors with undefs to the same size as the mask.
>> -    unsigned NumConcat = MaskNumElts / Src1NumElts;
>> -    std::vector<SDValue> UnOps(Src1NumElts,
>> -                               DAG.getNode(ISD::UNDEF,
>> -
>> VT1.getVectorElementType()));
>> -    SDValue UndefVal = DAG.getNode(ISD::BUILD_VECTOR, VT1,
>> -                                   &UnOps[0], UnOps.size());
>> +    // Pad both vectors with undefs to make them the same length as
>> the mask.
>> +    unsigned NumConcat = MaskNumElts / SrcNumElts;
>> +    SDValue UndefVal = DAG.getNode(ISD::UNDEF, SrcVT);
>>
>>    SmallVector<SDValue, 8> MOps1, MOps2;
>> -    MOps1.push_back(V1);
>> -    MOps2.push_back(V2);
>> +    MOps1.push_back(Srcs[0]);
>> +    MOps2.push_back(Srcs[1]);
>>    for (unsigned i = 1; i != NumConcat; ++i) {
>>      MOps1.push_back(UndefVal);
>>      MOps2.push_back(UndefVal);
>>    }
>
> It seems silly to use vectors instead of arrays here.

We could use a dynamically allocated array via new.

>>>> [Deleted Text]
>>
>>
>>
>> +        int Idx = cast<ConstantSDNode>(Arg)->getZExtValue();
>> +        int Input = 0;
>> +        if (Idx >= SrcNumElts) {
>> +          Input = 1;
>> +          Idx -= SrcNumElts;
>>        }
>> -        Mask = DAG.getNode(ISD::BUILD_VECTOR, Mask.getValueType(),
>> -                           &MappedOps[0], MappedOps.size());
>> +        if (Idx > MaxRange[Input])
>> +          MaxRange[Input] = Idx;
>> +        if (Idx < MinRange[Input])
>> +          MinRange[Input] = Idx;
>> +      }
>> +    }
>>
>> -        setValue(&I, DAG.getNode(ISD::VECTOR_SHUFFLE, VT, V1, V2,
>> Mask));
>> -        return;
>> +    // Check if the access is smaller than the vector size and can
>> we find
>> +    // a reasonable extract index.
>> +    int RangeUse[2];  // 0 = Unused, 1 = Extract, 2 = Can not
>> Extract.
>
> Perhaps initialize RangeUse with 2's to eliminate some nesting below?
>

That make sense.  I can remove a few cases.

>>
>> +    int StartIdx[2];  // StartIdx to extract from
>> +    for (int Input=0; Input < 2; ++Input) {
>
> int -> unsigned?
>
See above.

Thanks for the comments,
   -- Mon Ping
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20081118/515bab4e/attachment.html>