[llvm-commits] [llvm] r59025 - in /llvm/trunk: include/llvm/CodeGen/SelectionDAGNodes.h lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp lib/CodeGen/SelectionDAG/LegalizeTypes.h

[Mon Ping Wang]

Hi Duncan,

On Nov 11, 2008, at 12:47 AM, Duncan Sands wrote:

> Hi,
>
>> +SDValue DAGTypeLegalizer::PromoteIntRes_CONVERT_RNDSAT(SDNode *N) {
>> +  ISD::CvtCode CvtCode = cast<CvtRndSatSDNode>(N)->getCvtCode();
>> +  assert ((CvtCode == ISD::CVT_SS || CvtCode == ISD::CVT_SU ||
>> +           CvtCode == ISD::CVT_US || CvtCode == ISD::CVT_UU ||
>> +           CvtCode == ISD::CVT_SF || CvtCode == ISD::CVT_UF) &&
>> +          "can only promote integers");
>
> to promote the result, you should only need to modify the return
> type of the node, right?  I'm not sure why it's helpful to look
> at the operand here (some promote methods look at the operand
> because they can then promote in a more optimal way).

I'm probably misunderstanding the design.  PromoteIntRes will just  
promote the result.   PromoteIntOp promotes an operand.  An operation  
that can have an integer result will need a PromteIntRes routine and  
any operation that can take an integer input will need a  
PromoteIntOp.  The only cases where PromoteIntRes routine will look at  
is operand is to know how to promote the result correctly. (e.g.,  bit  
convert  needs to know what the input operand is to know what to  
generate).

>> +  case PromoteInteger:
>> +    return DAG.getConvertRndSat(OutVT, GetPromotedInteger(InOp),
>
> Also, promoted integers in general have rubbish in the extra bits.
> So suppose originally you were converting i8 to i16, and i8 and
> i16 get promoted to i32.  Then here you will convert an i32 with
> the original i8 in the lowest 8 bits, but with garbage in the upper
> 24 bits, into an i32.  This may give wrong results.  In general
> you need to do a SIGN_EXTEND_INREG or DAG.ZeroExtendInReg on the
> promoted value to ensure that the extra bits are as desired.

I didn't do the sign extensions or zero extensions because how these  
convert operate.  If the convert is i8 to a float, it will do the  
operation on the lower 8 bits and ignore the rest so having junk up  
there is fine.  The issue is that I shouldn't look at the promoted  
value.

> Ciao,
>
> Duncan.
>
> PS:
>
>> +  case ISD::CONVERT_RNDSAT: {
>> +    SDValue Op0 = ScalarizeVectorOp(Node->getOperand(0));
>> +    Result = DAG.getConvertRndSat(NewVT, Op0,
>> +                                  DAG.getValueType(NewVT),
>> +                                  DAG.getValueType(Op0.getValueType 
>> ()),
>> +                                  Node->getOperand(3),
>> +                                  Node->getOperand(4),
>> +
>> cast<CvtRndSatSDNode>(Node)->getCvtCode()); +    break;
>
> For CONVERT_RNDSAT on vectors, how about storing the vector element  
> types
> as src and dest types, rather than the vector types.  So a  
> CONVERT_RNDSAT
> on [4 x i32], turning into [4 x i16] would have src = i16 and dest =  
> i32.
> This seems conceptually a bit simpler to me, and also you wouldn't  
> have
> to recalculate src and dest types here and in split.

That is a good idea.  The convert operator should store the kind of  
conversion that is being done.  Storing the vector in the node doesn't  
provide any additional useful information.

Thanks,
   -- Mon Ping