<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Evan,<div><br><div><div>On Nov 18, 2008, at 10:00 AM, Evan Cheng wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Hi Mon Ping,<br><br>Thanks. Some nitpicks below.<br><br>Evan<br><br>On Nov 15, 2008, at 9:06 PM, Mon P Wang wrote:<br><br><blockquote type="cite">[deleted text]</blockquote></div></blockquote><blockquote type="cite"><div><blockquote type="cite"><font class="Apple-style-span" color="#000000"><br></font></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">void SelectionDAGLowering::visitShuffleVector(User &I) {<br></blockquote><blockquote type="cite">- SDValue V1 = getValue(I.getOperand(0));<br></blockquote><blockquote type="cite">- SDValue V2 = getValue(I.getOperand(1));<br></blockquote><blockquote type="cite">+ SDValue Srcs[2];<br></blockquote><blockquote type="cite">+ Srcs[0] = getValue(I.getOperand(0));<br></blockquote><blockquote type="cite">+ Srcs[1] = getValue(I.getOperand(1));<br></blockquote><br>A common idiom used is:<br>SDValue Srcs[] = { getValue(I.getOperand(0), <br>getValue(I.getOperand(1)) };<br><br>Is an array preferrable to V1 and V2?<br><br></div></blockquote><div><br></div><div>Probably not. Src1 and Src2 is preferable to V1 and V2 as it is a little more descriptive. I'll make the change.</div><br><blockquote type="cite"><div><br><blockquote type="cite"><br></blockquote><blockquote type="cite"> SDValue Mask = getValue(I.getOperand(2));<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> MVT VT = TLI.getValueType(I.getType());<br></blockquote><blockquote type="cite">- MVT VT1 = V1.getValueType();<br></blockquote><blockquote type="cite">- unsigned MaskNumElts = Mask.getNumOperands();<br></blockquote><blockquote type="cite">- unsigned Src1NumElts = VT1.getVectorNumElements();<br></blockquote><blockquote type="cite">+ MVT SrcVT = Srcs[0].getValueType();<br></blockquote><blockquote type="cite">+ int MaskNumElts = Mask.getNumOperands();<br></blockquote><blockquote type="cite">+ int SrcNumElts = SrcVT.getVectorNumElements();<br></blockquote><br>Why int instead of unsigned?<br><br></div></blockquote><div><br></div><div>When I was checking the range for the indexes, I set the values to be out of range, i.e., -1 and SrcNumElts. This makes the range a integer and since I compare with SrcNumElts, I would be doing a signed to unsigned comparison. Making them all int avoid this.</div><div><br></div><blockquote type="cite"><div><blockquote type="cite">[Deleted Text]</blockquote><blockquote type="cite"><span class="Apple-style-span" style="color: rgb(0, 0, 0); ">+</span></blockquote><blockquote type="cite">+ if (SrcNumElts < MaskNumElts && MaskNumElts % SrcNumElts == 0) {<br></blockquote><blockquote type="cite">+ // Mask is longer than the source vectors and is a multiple of <br></blockquote><blockquote type="cite">the source<br></blockquote><blockquote type="cite">+ // vectors. We can use concatenate vector to make the mask and <br></blockquote><blockquote type="cite">vectors<br></blockquote><blockquote type="cite">+ // length match.<br></blockquote><br>lengthes.<br><br></div></blockquote><div><br></div>I'll fix this.</div><div><br><blockquote type="cite"><div><blockquote type="cite"><span class="Apple-style-span" style="color: rgb(0, 0, 0); ">+ if (SrcNumElts*2 == MaskNumElts && SequentialMask(Mask, 0)) {</span></blockquote><blockquote type="cite">+ // The shuffle is concatenating two vectors together.<br></blockquote><blockquote type="cite">+ setValue(&I, DAG.getNode(ISD::CONCAT_VECTORS, VT, Srcs[0], <br></blockquote><blockquote type="cite">Srcs[1]));<br></blockquote><blockquote type="cite"> return;<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">- // Pad both vectors with undefs to the same size as the mask.<br></blockquote><blockquote type="cite">- unsigned NumConcat = MaskNumElts / Src1NumElts;<br></blockquote><blockquote type="cite">- std::vector<SDValue> UnOps(Src1NumElts,<br></blockquote><blockquote type="cite">- DAG.getNode(ISD::UNDEF,<br></blockquote><blockquote type="cite">- <br></blockquote><blockquote type="cite">VT1.getVectorElementType()));<br></blockquote><blockquote type="cite">- SDValue UndefVal = DAG.getNode(ISD::BUILD_VECTOR, VT1,<br></blockquote><blockquote type="cite">- &UnOps[0], UnOps.size());<br></blockquote><blockquote type="cite">+ // Pad both vectors with undefs to make them the same length as <br></blockquote><blockquote type="cite">the mask.<br></blockquote><blockquote type="cite">+ unsigned NumConcat = MaskNumElts / SrcNumElts;<br></blockquote><blockquote type="cite">+ SDValue UndefVal = DAG.getNode(ISD::UNDEF, SrcVT);<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> SmallVector<SDValue, 8> MOps1, MOps2;<br></blockquote><blockquote type="cite">- MOps1.push_back(V1);<br></blockquote><blockquote type="cite">- MOps2.push_back(V2);<br></blockquote><blockquote type="cite">+ MOps1.push_back(Srcs[0]);<br></blockquote><blockquote type="cite">+ MOps2.push_back(Srcs[1]);<br></blockquote><blockquote type="cite"> for (unsigned i = 1; i != NumConcat; ++i) {<br></blockquote><blockquote type="cite"> MOps1.push_back(UndefVal);<br></blockquote><blockquote type="cite"> MOps2.push_back(UndefVal);<br></blockquote><blockquote type="cite"> }<br></blockquote><br>It seems silly to use vectors instead of arrays here.<br></div></blockquote><div><br></div><div>We could use a dynamically allocated array via new.</div><div><br></div><blockquote type="cite"><div><blockquote type="cite"><span class="Apple-style-span" style="color: rgb(0, 0, 0); "><blockquote type="cite"><div><blockquote type="cite">[Deleted Text]</blockquote></div></blockquote></span><div><br></div></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">+ int Idx = cast<ConstantSDNode>(Arg)->getZExtValue();<br></blockquote><blockquote type="cite">+ int Input = 0;<br></blockquote><blockquote type="cite">+ if (Idx >= SrcNumElts) {<br></blockquote><blockquote type="cite">+ Input = 1;<br></blockquote><blockquote type="cite">+ Idx -= SrcNumElts;<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite">- Mask = DAG.getNode(ISD::BUILD_VECTOR, Mask.getValueType(),<br></blockquote><blockquote type="cite">- &MappedOps[0], MappedOps.size());<br></blockquote><blockquote type="cite">+ if (Idx > MaxRange[Input])<br></blockquote><blockquote type="cite">+ MaxRange[Input] = Idx;<br></blockquote><blockquote type="cite">+ if (Idx < MinRange[Input])<br></blockquote><blockquote type="cite">+ MinRange[Input] = Idx;<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">- setValue(&I, DAG.getNode(ISD::VECTOR_SHUFFLE, VT, V1, V2, <br></blockquote><blockquote type="cite">Mask));<br></blockquote><blockquote type="cite">- return;<br></blockquote><blockquote type="cite">+ // Check if the access is smaller than the vector size and can <br></blockquote><blockquote type="cite">we find<br></blockquote><blockquote type="cite">+ // a reasonable extract index.<br></blockquote><blockquote type="cite">+ int RangeUse[2]; // 0 = Unused, 1 = Extract, 2 = Can not <br></blockquote><blockquote type="cite">Extract.<br></blockquote><br>Perhaps initialize RangeUse with 2's to eliminate some nesting below?<br><br></div></blockquote><div><br></div>That make sense. I can remove a few cases.</div><div><br><blockquote type="cite"><div><blockquote type="cite"><br></blockquote><blockquote type="cite">+ int StartIdx[2]; // StartIdx to extract from<br></blockquote><blockquote type="cite">+ for (int Input=0; Input < 2; ++Input) {<br></blockquote><br>int -> unsigned?<br><br></div></blockquote>See above.</div><div><br></div><div>Thanks for the comments,</div><div> -- Mon Ping</div></div></body></html>