[llvm-commits] Patch: updated widen in LegalizeType
Mon Ping Wang
monping at apple.com
Tue Dec 16 01:35:15 PST 2008
Hi Duncan,
On Dec 15, 2008, at 12:28 AM, Duncan Sands wrote:
> 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.
>
You are right. I replaced it with a simple test for checking for
power of 2 instead of finding the value of 2.
>> + // vector length is a power of 2 - split to half the size
> vector -> Vector
> Missing full stop at end of line.
>
Fixed.
>> + 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?
>
The logic in the order is based on increasing vector size and within
vectors of the same size, we list them in increase vector element
size. The vectors v3i32 and v3f32 were original there and I left them
as I assume they were a useful type (graphics like v3f32 but I'm not
sure if there is a machine that can process that vector size).
>> + 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
>
Fixed.
>> + 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.
>
Yes, I moved it to a separate patch recently as I wanted the widening
patch to deal only with widening.
>> + 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.
>
Yes, this can only occur if we widened the input. I moved it inside.
>> + 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.
>
Because widening and splitting are opposite operations, we can run
into problems when creating new nodes. Consider the the case when we
widen a conversion from v2f64 to v2i32. The widen type is v4i32 so we
widen the operation and widen the input to v4f64 by using
concat_vectors. We place this new node into the worklist and return
to check if the nodes are legal. We first encounter the
concat_vectors with an illegal type v4f64 so we split it down to
v2f64. This will require updating the result of the conversion node
to v2i32 again and we add the nodes to the work list and do it all
over again. This is only a problem for operations with different
input and output types.
The code in WidenVecRes_Convert that does concatenation is useful if
an architecture has multiple legal vector with different size (e.g.,
if v4i32 and v4f64 are legal). There is some more work here that
should be done to improve performance.
>> + if (InVTNumElts % WidenNumElts == 0) {
>
> How can this happen?
Assuming v16i8 is a legal type and we are doing a conversion from v6i8
to v6i16. The v6i16 will be widen to v8i16 and legal type for v6i8 is
v16i8 so we would widen to that.
>> + unsigned MinElts = std::min(InVTNumElts, WidenNumElts);
>
> How can this ever be different to InVTNumElts?
>
See above.
>> + 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).
>
The missing break is a bug. If the input is promoted but doesn't
match the required size, we would need to properly widen the promoted
input to do the conversion. I'll add a test for this case.
>> + 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;.
>
Done.
>> + 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?
>
You are correct and for X86, it is hard to create a test case for this
because when we widen two different vector types, we will typically
widen them to the same size and never hit this case. I'll try to see
if I'm converting an integer to a vector and see if I can hit 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.
I'll add that.
>
>
>> + 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.
>
I'll change this and I'll add widening support to PerformExpensive
checks.
>> + unsigned WidenNumElts = WidenVT.getVectorNumElements();
>> +
>> + SmallVector<SDValue, 16> NewOps(N->op_begin(), N->op_end());
>
> How about reserving space for WidenNumElts?
Done.
>
>
>> +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.
>
Done.
>> +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.
>
It looks like I misunderstood SELECT_CC. I thought that the true and
false values can be vectors if the comparions values are vectors but
looking at what split does, they are not. The getNode doesn't check
if the true and false values are not vectors. None of my test cases
hits this instruction.
>> + // Readjust mask based on new input vector length.
>
> Readjust -> Adjust
>
Done.
>> +SDValue DAGTypeLegalizer::WidenVecRes_VSETCC(SDNode *N) {
> ...
>> + assert(InVT.isVector() && "can not widen non vector type");
>
> How can there be a non-vector type here?
>
It has to be a vector so the assert is unnecessary.
>> + InOp1 = GetWidenedVector(InOp1);
>
> Why should the operands widen? The result has a different
> vector type, and it is the result that you know widens.
>
For VSETCC, the input vectors must match the same length as the
output. VSETCC is used for vectors and the most common definition for
vectors is that the input vectors and the result vectors have the same
size for the element and the number of elements so they will widen to
the same amount usually (this has been so far true for the code we
generate).
>> + // 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.
>
Done when I updated to TOT.
>> +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.
>
Done.
>> +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.
>
I'll do so.
>
>> +// Vector Widening Utilities
>
> I was too tired to review these. However at first glance they seem
> rather messy, with millions of parameters etc.
>
This currently uses a recursive algorithm that passes the store
information across to chop it off. I'm planning to replace with an
iterative one during the break so these should go away at some point.
>> +/// 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?
>
The only case we would narrow if an input is widened to a vector type
with more elements than the result because they are different types.
I'll change the name.
>> 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.
>
Fixed.
>> 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 :)
>
PR2957 is starting to annoy me. I had to weaken the check
inSelectionDAGLegalize::HandleOp that checks for legal types because
build_vector masks can be illegal. I'll try to look into this during
the break.
>> + /// 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?
>
I tried covered a lot of cases and try to avoid the overlap from the
test cases that we had for vectors but obviously, I didn't hit every
code path. I did run our tests with vectors over it.
Thanks you very much for the time and effort for the review.
-- Mon Ping
More information about the llvm-commits
mailing list