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

[Duncan Sands]

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

> +  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.

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.