[llvm-commits] Patch: LegalizeType for promoting some operands

[Mon Ping Wang]

Hi Duncan,

On Dec 14, 2008, at 9:11 PM, Duncan Sands wrote:

> Hi Mon Ping,
>
>> -  return DAG.getNode(ISD::SELECT, NewVT, Odd, Hi, Lo);
>> +  SDValue Res = DAG.getNode(ISD::SELECT, NewVT, Odd, Hi, Lo);
>> +  MVT NVT = TLI.getTypeToTransformTo(OldVT);
>> +  if (Res.getValueType() != NVT) {
>> +    assert(Res.getValueType().getSizeInBits() < NVT.getSizeInBits 
>> () &&
>> +           "Unexpected Result Type");
>> +    Res = DAG.getNode(ISD::ANY_EXTEND, NVT, Res);
>> +  }
>
> you might as well not bother testing Res.getValueType() != NVT, and
> always do the extension.  If the types are equal then SelectionDAG
> will fold the node and simply return Res.  The assertion is also
> useless: this is checked by the getNode call.
>

I have removed this test.

>> +  MVT OldEVT = Val.getValueType();
>
> Please add an assertion that this is the same as the vector element
> type.
>
Done.

>> +  // BitConvert a vector of twice the length out of the expanded  
>> elements,
>
> -> Bitconvert to a vector of twice the length with elements of the  
> expanded type,
>
>> +  SDValue Idx = N->getOperand(2);
>> +  if (ConstantSDNode *CIdx = dyn_cast<ConstantSDNode>(Idx)) {
>> +    unsigned IdxVal = CIdx->getZExtValue()*2;
>> +    NewVec = DAG.getNode(ISD::INSERT_VECTOR_ELT, NewVecVT, NewVec,  
>> Lo,
>> +                         DAG.getIntPtrConstant(IdxVal));
>> +    NewVec =  DAG.getNode(ISD::INSERT_VECTOR_ELT, NewVecVT,  
>> NewVec, Hi,
>> +                          DAG.getIntPtrConstant(IdxVal + 1));
>> +  } else {
>> +    Idx = DAG.getNode(ISD::SHL, Idx.getValueType(), Idx,
>> +                      DAG.getIntPtrConstant(1));
>> +    NewVec = DAG.getNode(ISD::INSERT_VECTOR_ELT, NewVecVT, NewVec,  
>> Lo,
>> Idx); +    Idx = DAG.getNode(ISD::ADD, Idx.getValueType(), Idx,
>> +                      DAG.getIntPtrConstant(1));
>> +    NewVec =  DAG.getNode(ISD::INSERT_VECTOR_ELT, NewVecVT,  
>> NewVec, Hi,
>> Idx); +  }
>
> I think you should drop the special version for constants, and keep
> the second version.  Also, in the second version it seems better to
> calculate 2*Idx by adding Idx to itself, rather than shifting left
> by 1.
>

Since DAGCombine will optimized the case of a constant, I remove the  
special version to keep the code cleaner.

>> +  for (unsigned i=1; i < NumElts; ++i)
>
> Missing spaces around the equals sign.
>

Done.

Thanks for the review,
   -- Mon Ping