[llvm-commits] Patch: updated widen in LegalizeType

[Duncan Sands]

Hi Mon Ping,

> +          return VT == VT.getPow2VectorType() ? Expand : Promote;

this seems like an expensive way of testing whether the the number
of elements is a power of two.

> +        // vector length is a power of 2 - split to half the size
vector -> Vector
Missing full stop at end of line.

> +      v2i8           =  14,   //  2 x i8
> +      v4i8           =  15,   //  4 x i8
> +      v2i16          =  16,   //  2 x i16
> +      v8i8           =  17,   //  8 x i8
> +      v4i16          =  18,   //  4 x i16
> +      v2i32          =  19,   //  2 x i32
> +      v1i64          =  20,   //  1 x i64
> +      v16i8          =  21,   // 16 x i8
> +      v8i16          =  22,   //  8 x i16
> +      v3i32          =  23,   //  3 x i32
> +      v4i32          =  24,   //  4 x i32
> +      v2i64          =  25,   //  2 x i64

What's the logic behind this order?  How about dropping
v3i32 and v3f32 from the list of simple value types?

> +  case WidenVector:
> +    if (OutVT.bitsEq(TLI.getTypeToTransformTo(InVT)))

This should be NInVT rather than TLI.getTypeToTransformTo(InVT).

> +      // The input widens to the same size.  Convert to the widen value.

widen value -> widened value

> +    case ISD::SCALAR_TO_VECTOR: Res = ExpandIntOp_SCALAR_TO_VECTOR(N);

Duplicate from your other patch.

> +SDValue DAGTypeLegalizer::ExpandIntOp_SCALAR_TO_VECTOR(SDNode *N) {

Duplicate from your other patch.

> +  unsigned Opcode = N->getOpcode();
> +  unsigned InVTNumElts = InVT.getVectorNumElements();
> +  if (InVTNumElts == WidenNumElts)
> +    return DAG.getNode(Opcode, WidenVT, InOp);

Don't you get here only if the test
  if (getTypeAction(InVT) == WidenVector) {
passed a few lines earlier?  If so, this early exit
would be better inside that previous logic.

> +  if (TLI.isTypeLegal(InWidenVT)) {
> +    // Because the result and the input are different vector types, widening
> +    // the result could create a legal type but widening the input might make
> +    // it an illegal type that might lead to repeatedly splitting the input
> +    // and then widening it. To avoid this, we widen the input only if
> +    // it results in a legal type.

I don't see why this is a problem for this code, which is legalizing the
result.  Isn't it really a problem for WidenVecOp_Convert or SplitVecOp_Convert?
They can refuse to turn a legal result type into an illegal one, instead falling
back to some evil BUILD_VECTOR.

> +    if (InVTNumElts % WidenNumElts == 0) {

How can this happen?

> +  unsigned MinElts = std::min(InVTNumElts, WidenNumElts);

How can this ever be different to InVTNumElts?

> +  case PromoteInteger:
> +    InOp = GetPromotedInteger(InOp);
> +    InVT = InOp.getValueType();
> +    if (WidenVT.bitsEq(InVT))
> +      // The InOp promotes to the same size.  Convert the promoted value.
> +      return DAG.getNode(ISD::BIT_CONVERT, WidenVT, InOp);

I don't think it's wise to modify InOp and InVT like this when
you are not sure to return.  It at least needs a comment in the
break case.  For that matter, it is missing a break (harmless
here, but better style to put one).

> +  case WidenVector:
> +    InOp = GetWidenedVector(InOp);
> +    InVT = InOp.getValueType();
> +    if (WidenVT.bitsEq(InVT))
> +      // The input widens to the same size. Convert to the widen value.
> +      return DAG.getNode(ISD::BIT_CONVERT, WidenVT, InOp);
> +  }

Likewise, including like of an explicit break;.

> +  if (WidenSize % InSize == 0) {
> +    MVT NewInVT;
> +    unsigned NewNumElts = WidenSize / InSize;
> +    if (InVT.isVector()) {
> +      MVT InEltVT = InVT.getVectorElementType();
> +      NewInVT= MVT::getVectorVT(InEltVT, InEltVT.getSizeInBits() / WidenSize);

Shouldn't this be WidenSize / InEltVT.getSizeInBits()?  Can you exercise
this code path with a testcase?  Isn't NewNumElts wrong in this case?

> +    } else
> +      NewInVT = MVT::getVectorVT(InVT, NewNumElts);

Missing braces { } around this line.

How about a comment here about the invariants that should hold for
NewInVT etc.

> +    if (TLI.isTypeLegal(NewInVT)) {
> +      // Because the result and the input are different vector types, widening
> +      // the result could create a legal type but widening the input might make
> +      // it an illegal type that might lead to repeatedly splitting the input
> +      // and then widening it. To avoid this, we widen the input only if
> +      // it results in a legal type.

Same comment as above: looks like this belongs in the bitconvert operand
legalization logic.

> +  // This should occur very rarely. Lower the bit-convert to a store/load
> +  // from the stack, then widen the load.
> +  SDValue Op = CreateStackStoreLoad(InOp, N->getValueType(0));
> +  return WidenVecRes_LOAD(cast<LoadSDNode>(Op.getNode()));

Sorry, but this will now cause a failure if you run llc with
enable-legalize-types-checking (I tightened up a bunch of
invariants).  For that matter, please add widening support
to DAGTypeLegalizer::PerformExpensiveChecks.

> +  unsigned WidenNumElts = WidenVT.getVectorNumElements();
> +
> +  SmallVector<SDValue, 16> NewOps(N->op_begin(), N->op_end());

How about reserving space for WidenNumElts?

> +SDValue DAGTypeLegalizer::WidenVecRes_CONCAT_VECTORS(SDNode *N) {

I was too tired to review this.

> +SDValue DAGTypeLegalizer::WidenVecRes_CONVERT_RNDSAT(SDNode *N) {

I don't plan to review this.

> +SDValue DAGTypeLegalizer::WidenVecRes_EXTRACT_SUBVECTOR(SDNode *N) {

I was too tired to review this.

> +SDValue DAGTypeLegalizer::WidenVecRes_INSERT_VECTOR_ELT(SDNode *N) {
> +  MVT WidenVT = TLI.getTypeToTransformTo(N->getValueType(0));
> +  SDValue InOp = GetWidenedVector(N->getOperand(0));
> +  assert(InOp.getValueType() == WidenVT);

Please don't use getTypeToTransformTo, and just get WidenVT as
InOp.getValueType() instead.

> +SDValue DAGTypeLegalizer::WidenVecRes_LOAD(SDNode *N) {

I was too tired to review this.

> +    MVT CondEltVT = CondVT.getVectorElementType();
> +    MVT CondWidenVT =  MVT::getVectorVT(CondEltVT, WidenNumElts);
> +    if (getTypeAction(CondVT) == WidenVector)
> +      Cond1 = GetWidenedVector(Cond1);
> +    if (Cond1.getValueType() != CondWidenVT)
> +       Cond1 = WidenToType(Cond1, CondWidenVT);

This second "if" can just be "else".  How about introducing
a utility function which gets a value of the widened type
using either GetWidenedVector or WidenToType?

> +  assert(CondVT.isVector() && "can not widen non vector type");

non vector -> non-vector

> +  assert(CondVT == Cond2.getValueType() && "mismatch lhs/rhs");

mismatch -> mismatched

> +  assert(Cond1.getValueType() == WidenCondVT &&
> +         Cond2.getValueType() == WidenCondVT && "condition not widen");

Why should the conditions widen?  In fact, why should the conditions
be vectors at all?  And if they are, is it valid to just extend them
with undefined values (via widening)?  For example, if the CC was Eq,
then after widening originally equal vectors may no longer be equal.

> +  assert(InOp1.getValueType() == WidenVT && InOp2.getValueType() ==
> WidenVT && +         "operands not widen");

Seems like a pointless assertion.  The fact that the types of these
match the result type should be checked in SelectionDAG.cpp.

> +  // Readjust mask based on new input vector length.

Readjust -> Adjust

> +SDValue DAGTypeLegalizer::WidenVecRes_VSETCC(SDNode *N) {
...
> +  assert(InVT.isVector() && "can not widen non vector type");

How can there be a non-vector type here?

> +  InOp1 = GetWidenedVector(InOp1);

Why should the operands widen?  The result has a different
vector type, and it is the result that you know widens.

> +  // If the result is N, the sub-method updated N in place.
> +  if (Res.getNode() == N) {
> +    // Mark N as new and remark N and its operands.  This allows us to
> correctly +    // revisit N if it needs another step of promotion and
> allows us to visit +    // any new operands to N.
> +    ReanalyzeNode(N);
> +    return true;
> +  }

I changed the way this stuff works.  Please copy the new logic from
one of the similar places.

> +SDValue DAGTypeLegalizer::WidenVecOp_Convert(SDNode *N) {
...
> +  SDValue InOp = N->getOperand(0);

If you use the widened vector instead, you avoid the need for the
legalizer to later widen the EXTRACT_VECTOR_ELT nodes you create.

> +SDValue DAGTypeLegalizer::WidenVecOp_CONCAT_VECTORS(SDNode *N) {
...
> +    SDValue InOp = N->getOperand(i);

Likewise it would be more efficient to use the already widened
vectors here.

> +SDValue DAGTypeLegalizer::WidenVecOp_STORE(SDNode *N) {

I was too tired to review this.  However it looks like you have
introduced truncating vector stores (and presumably extending vector
loads).  If so, you should extensively document this.


> +// Vector Widening Utilities

I was too tired to review these.  However at first glance they seem
rather messy, with millions of parameters etc.

> +/// Given a vector input, widens (narrows) it to a vector of WidenVT.  The

what does (narrows) mean?  Should it be: widens or narrows?  If so, please
change the name to something like TakeWidthFromType.  Also, isn't it likely
that a need to narrow represents a bug?

>          SplitVectorResult(N, i);
>          Changed = true;
>          goto NodeDone;
> +      case WidenVector:
> +        WidenVectorResult(N, i);
> +        goto NodeDone;

You forgot to set Changed to true.

> +      case WidenVector:
> +        NeedsRevisit = WidenVectorOperand(N, i);
> +        break;

Likewise.

>    bool IgnoreNodeResults(SDNode *N) const {
> -    return N->getOpcode() == ISD::TargetConstant;
> +    return N->getOpcode() == ISD::TargetConstant ||
> +           IgnoredNodesResultsSet.count(N);

This is a temporary measure until PR2957 is fixed.  Please add a
comment stating this.  Even better, how about fixing PR2957 :)

> +  /// WidenVectors - For vector nodes that need to be widened, this map
> +  /// which widen value to use.

-> For vector nodes that need to be widened, indicates the widened value to use.

> +  SDValue ExpandIntOp_SCALAR_TO_VECTOR(SDNode *N);

Duplicate from your other patch.

> +    assert(WidenedOp.getNode() && "Operand wasn't Widened?");

wasn't Widened -> wasn't widened

Finally, I think you should apply your patch (with cleanups): while there
are further improvements that can be made, at this point I think it is best
to get it into the tree.  Don't forget to add testcases.  Can you get complete
coverage?

Ciao,

Duncan.