[llvm-commits] [llvm] r59001 - in /llvm/trunk: include/llvm/CodeGen/SelectionDAG.h include/llvm/CodeGen/SelectionDAGNodes.h include/llvm/Intrinsics.td lib/CodeGen/SelectionDAG/LegalizeDAG.cpp lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp lib/CodeGen/SelectionDAG/LegalizeTypes.h lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp lib/CodeGen/SelectionDAG/SelectionDAG.cpp lib/CodeGen/SelectionDAG/SelectionDAGBuild.cpp lib/Target/TargetSelectionDAG.td

[Mon Ping Wang]

Hi,

On Nov 10, 2008, at 1:16 PM, Duncan Sands wrote:

> Hi,
>
>> +    //   1) dest type (type to convert to)
>
> are you really storing this in the node?  That's not needed because
> it is the type of the node result.
>
>> +    //   2) src type (type to convert from)
>
> Likewise for this.  This must be the type of "0) value".

The way this node was implemented was to keep track of the original  
source and destination type to indicate the type of convert to do.   
After legalization, the source and destination type may change (e.g.,  
i8 to i32 on X86 since i32 is legal type) but we don't want to change  
the operation of the conversion, e.g., if one is doing an float to  
unsigned i8, the promotion will make the destination a i32 but we want  
to keep the operation as converting a floating point to an i8.


>> +    //   3) rounding imm
>
> What is this precisely?
>

It is an immediate that specifies the rounding mode when dealing with  
floating point, i.e., round to nearest even, round towards zero, round  
toward positive infinity, round towards negative infinity.  It is the  
same as FLT_ROUNDS_.

>> +    //   4) saturation imm
>
> What is this precisely?

It is an immediate that specifies the saturation.  For integer dest  
types, it clamps the result to the range of the destination. For  
example,  converting a floating point number 8.0E10 to a 8 bit integer  
will clamp to 127.

Effectively, argument 3 and 4 should be considered not as normal  
arguments but like the opcode, they are modes to set what the argument  
will do. I was thinking that they should never be promoted.

>
>
>> +SDValue DAGTypeLegalizer::PromoteIntOp_CONVERT_RNDSAT(SDNode *N) {
>> +  MVT OutVT = TLI.getTypeToTransformTo(N->getValueType(0));
>
> Why is this OutVT?  It is the new type for the input, so if anything
> it should be InVT.  More traditional is NVT.
>> +  SDValue In = DAG.getConvertRndSat(OutVT,N->getOperand(0),
>
> You said the type of the input changed (OutVT) but did change the  
> input
> (still using N->getOperand(0)).  You should use: GetPromotedInteger 
> (N->getOperand(0))
>
> Also, what if it's operand 3 or 4 that needs promoting?
>

This is a bug.  We should only need to promote the result.  I don't  
think we ever need to promote an operand.


>> +SDValue DAGTypeLegalizer::ScalarizeVecRes_CONVERT_RNDSAT(SDNode  
>> *N) {
>
> The documentation forgot to mention that CONVERT_RNDSAT can be  
> applied to
> vectors.
>
I added that.


>> +  return DAG.getConvertRndSat(NewVT, Op0, DAG.getValueType(NewVT),
>> +                              DAG.getValueType(Op0.getValueType()),
>
> ^ Here you see that several arguments to getConvertRndSat are
> (apparently) redundant.
>
>> +  GetSplitDestVTs(N->getValueType(0), LoVT, HiVT);
>> +  SDValue VLo, VHi;
>> +  GetSplitVector(N->getOperand(0), VLo, VHi);
>
> There's no need to call GetSplitDestVTs: LoVT is the type of
> VLo, HiVT is the type of VHi.
>

LoVT and HiVT should split the vector based on the destination type  
while the VLo and VHi are split vector of the source which can be a  
different type.

>> +  SDValue DTyOpLo =  DAG.getValueType(LoVT);
>> +  SDValue DTyOpHi =  DAG.getValueType(HiVT);
>> +  SDValue STyOpLo =  DAG.getValueType(VLo.getValueType());
>> +  SDValue STyOpHi =  DAG.getValueType(VHi.getValueType());
>
> ^ None of these would be needed if getConvertRndSat didn't take
> redundant arguments.
>

This node should have a limited lifetime and clients should avoid it  
as it will be redesign.  It functionality will probably be subsumed  
with how SINT_TO_FP, FP_ROUND, etc.. works after we add saturation and  
support for vector types.

Thanks for your comments,
-- Mon Ping