<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Duncan,<div><br><div><div>On Oct 30, 2008, at 2:38 AM, Duncan Sands wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Hi Mon Ping,<br><br><blockquote type="cite"> LegalizeAction getTypeAction(MVT VT) const {<br></blockquote><blockquote type="cite"> if (VT.isExtended()) {<br></blockquote><blockquote type="cite">- if (VT.isVector()) return Expand;<br></blockquote><blockquote type="cite">+ if (VT.isVector()) {<br></blockquote><blockquote type="cite">+ // First try vector widening<br></blockquote><blockquote type="cite">+ return Promote;<br></blockquote><blockquote type="cite">+ }<br></blockquote><br>is the plan to do something like what is done for arbitrary<br>precision integers: first promote to a type which is a power-of-two<br>in length, then (if that type is not legal) expand until reaching<br>a legal type. In the context of vectors I guess I'm asking if you<br>plan to do: v10i32 -> v16i32 -> two v8i32 -> four v4i32? Only the<br>last of these is a simple value type, so the above getTypeAction<br>logic will return "Promote" for all of them, even those you want<br>to split like the v16i32... In fact aren't you going to widen<br>such vectors for ever (infinite loop)?<br><br></div></blockquote><div><br></div><div>The intention is that if there we will only promote if there is a legal wider type. Since there is no legal V16i32, we will not try to promote directly to that so we won't go into a infinite loop. Ideally, given a v10i32 with v4i32 being legal on the machine, we first split it down to two v4i32 and a v2i32. The v2i32 can then be promoted to v4i32 and thus we end up with three v4i32. I discuss this a little more below.</div><br><blockquote type="cite"><div><blockquote type="cite">+ /// When and were to widen is target dependent based on the cost of<br></blockquote><br>When and were -> When and where<br><br><blockquote type="cite">+ /// WidenNodes - For nodes that need to be widen from one vector type to<br></blockquote><br>need to be widen -> need to be widened<br><br><blockquote type="cite">+ /// another, this contains the mapping of ones we have already widen. This<br></blockquote><br>of ones we have already widen -> of those we have already widened<br><br><blockquote type="cite">+ void AddWidenOperand(SDValue From, SDValue To) {<br></blockquote><br>AddWidenOperand -> AddWidenedOperand<br><br></div></blockquote>I'll fix these syntax problems and the ones you found below.</div><div><br><blockquote type="cite"><div><blockquote type="cite">+ /// WidenVectorOp - Widen a vector operation in order to do the computation<br></blockquote><blockquote type="cite">+ /// in a wider type given to a wider type given by WidenVT (e.g., v3i32 to<br></blockquote><br>This sentence does not parse for me...<br><br></div></blockquote><div><br></div><div>I clean it up with something like</div><div> Widen a vector operation to a legal type given by WidenVT</div><br><blockquote type="cite"><div><blockquote type="cite">+ /// v4i32). The produced value will have the correct value for the existing<br></blockquote><blockquote type="cite">+ /// elements but no guarantee is made about the new elements at the end of<br></blockquote><blockquote type="cite">+ /// the vector: it may be zero, sign-extended, or garbage. This is useful<br></blockquote><br>What do zero-extension and sign-extension mean in this context?<br><br></div></blockquote><div><br></div>What I should have wrote that it may be filled with 0s, 1s, or garbage. <br><br><blockquote type="cite"><div><blockquote type="cite">+ /// when we have instruction operating on an illegal vector type and we want<br></blockquote><br>have instruction -> have an instruction<br><br><blockquote type="cite">+ /// to widen it to do the computation on a legal wider vector type.<br></blockquote><br>In general the wider type will not be legal. For example when the vector type<br>is an extended type.<br><br><blockquote type="cite">+ /// Useful 16 element vector used to pass operands for widening<br></blockquote><br>16 element vector -> 16 element vector type<br>Missing full stop at end of line.<br><br><blockquote type="cite">+ /// LoadWidenVectorOp - Load a vector for a wider type. Returns true if<br></blockquote><br>I will comment fully on the implementation once you submit it for LegalizeTypes.<br><br><blockquote type="cite">+ case ISD::CONCAT_VECTORS: {<br></blockquote><br>Is this for widening?<br><br></div></blockquote><div><br></div>That slipped in. This is needed for the vector shuffle patch.<br><br><blockquote type="cite"><div><blockquote type="cite">+ // Fall thru to expand for vector<br></blockquote><br>That doesn't seem wise: if the operation is "widen" surely you have<br>to always widen rather than sometimes expand? After all, users of this<br>node are going to expect to find a widened value in the WidenedNodes<br>map, but it may not be there.</div></blockquote><div><br></div><div>What I should have done is rewrite <span class="Apple-style-span" style="color: rgb(32, 90, 94); font-family: Monaco; font-size: 10px; ">getTypeAction <span class="Apple-style-span" style="color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; ">in TargetLowering to check the TLI is possible for widening of that vector type. At the time, for some strange reason, I thought that function we were calling that was wrapped in the class and I didn't have access to the TLI. This will map exactly what will be done in LegalizeTypes.</span></span></div><div><br></div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"><br></font><blockquote type="cite">+ // Check if we have widen this node with another value<br></blockquote><blockquote type="cite">+ std::map<SDValue, SDValue>::iterator I =<br></blockquote><blockquote type="cite">+ WidenNodes.find(ST->getValue());<br></blockquote><br>Speak of the devil! Here is someone who expected to find a widened<br>node but doesn't always! I hope this is just a temporary measure<br>because widening support does not yet exist for all operations...</div></blockquote><div><br></div><div>This is due to the issue above. I can get rid of this case once I fixed it.</div><div><br></div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"><br></font><br><blockquote type="cite">+ // When widen is called, it is assumed that it is more efficient to use a<br></blockquote><blockquote type="cite">+ // wide type. The default action is to widen to operation to a wider legal<br></blockquote><blockquote type="cite">+ // vector type and then do the operation if it is legal by calling LegalizeOp<br></blockquote><blockquote type="cite">+ // again. If there is no vector equivalent, we will unroll the operation, do<br></blockquote><blockquote type="cite">+ // it, and rebuild the vector. If most of the operations are vectorizible to<br></blockquote><blockquote type="cite">+ // the legal type, the resulting code will be more efficient. If this is not<br></blockquote><blockquote type="cite">+ // the case, the resulting code will preform badly as we end up generating<br></blockquote><blockquote type="cite">+ // code to pack/unpack the results. It is the function that calls widen<br></blockquote><blockquote type="cite">+ // that is responsible for seeing this doesn't happen. For some cases, we<br></blockquote><blockquote type="cite">+ // have decided that it is not worth widening so we just split the operation.<br></blockquote><br>I don't like this roll-back strategy at all... Do you have some examples where</div></blockquote><blockquote type="cite"><div>it is better to split than to widen?<br><br></div></blockquote><div><br></div><div>The last sentence is incorrect. When we decide to widen, we should always widen. For operations where a vector operation doesn't exist, we will generate some nasty scalar code and the rebuild the vector.</div><br><blockquote type="cite"><div><blockquote type="cite">+ // Promote can mean<br></blockquote><blockquote type="cite">+ // 1) On integers, it means to promote type (e.g., i8 to i32)<br></blockquote><br>"For integers, it means use the promoted integer type (e.g. i8 to i32)."<br><br><blockquote type="cite">+ // 2) For vectors, it means try to widen (e.g., v3i32 to v4i32)<br></blockquote>"For vectors, it means use the widened vector type (e.g. v3i32 to v4i32)"<br><br>I don't think there should be any "try" about it: if the type action says<br>widen then it should be widened.<br><br></div></blockquote><div><br></div><div>The issue occurs for the "Extended Vectors" where we don't have a known vector types coming in. <span class="Apple-style-span" style="color: rgb(59, 130, 136); font-family: Monaco; font-size: 10px; ">ValueTypeActions<span style="color: #000000">.</span><span style="color: #205a5e">getTypeAction</span><span style="color: #000000">(</span>VT<span style="color: #000000">) <span class="Apple-style-span" style="font-family: Helvetica; font-size: 12px; ">will return "promote" for this case as it doesn't have access to the TLI to see if there is a type we can widen to. In LegalizeType/TargetLowering <span class="Apple-style-span" style="font-family: Monaco; font-size: 10px; ">getTypeAction<span class="Apple-style-span" style="font-family: Helvetica; font-size: 12px; ">, we should check the TLI to decide if we can widen and if so, return that we are widening.</span></span></span></span></span></div><br><blockquote type="cite"><div><blockquote type="cite">+ return PromoteInteger;<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite">+ else {<br></blockquote><br>-> } else {<br><br><blockquote type="cite">+/// getWidenVectorType: given a vector type, returns the type to widen to<br></blockquote><br>getWidenVectorType -> getWidenedVectorType<br><br><blockquote type="cite">+/// If there is no vector type that we want to widen to, returns MVT::Other<br></blockquote><br>Missing full stop at end of line.<br>Also, I don't think this information about whether we want to widen or<br>not should be here. The type action should say whether it is to be<br>widened or to be split. Probably this routine should simply always<br>return the vector type obtained by (1) if there is a legal vector type<br>with the same element type but more elements, return the smallest such<br>vector type; (2) otherwise, round the number of elements up to the<br>next power of two. By the way, if the number of elements is already<br>a power of two and there is no wider legal vector type, then you really<br>want to split not widen, so I think this routine should assert in that<br>case.<br><br></div></blockquote><div><br></div><div><br></div><div>My logic for getTypeAction is that it will get promote and then check the TLI to see if a legal vector type exist. If it doesn't, it should return split or scalarize. Otherwise, it will widen. The check to TLI is redundant for known types defined in ValueType as we already know if a legal type exist to widen to. Any client other than getTypeAction shouldn't need to check for MVT::Other because once we are trying to widen, the type must always exist.<div><br></div><div>So if we had vector of 3 elements and there is no valid 4 element vector type, we should just split and not try the next power of 2 and then split down to 4 operations. The rational that I see for going to the power of 2 is that is more likely that the split logic (where we split in 1/2) has a higher chance of finding the correct legal type. The down side is that if we go to a power of 2, we might end up doing work that doesn't benefit us. Given the example above of v10i32, if we promote to v16i32, we will break it down to 4 v4i32 where one of the v4i32 is not useful. With what I have done, we don't do any better, i.e., v10xi32 to two v5xi32 to "two v3xi32 and two v2xi32" that ends up to four v4i32. What would be better is to beef up the split logic to see what is the largest legal vector for that element type and try to split based on that.</div></div><div><br></div><div><br></div><blockquote type="cite"><div><blockquote type="cite">+/// When and were to widen is target dependent based on the cost of<br></blockquote><br>When and were -> When and where<br><br><blockquote type="cite">+ // First set operation action for all vector types to either to promote<br></blockquote><br>to either to -> to either<br><br><blockquote type="cite">+ // (for widening) or expand (for scalarization). Then we will selectively<br></blockquote><br>or expand (for scalarization) -> or expand (for scalarization or splitting)<br><br><blockquote type="cite">+ setOperationAction(ISD::EXTRACT_SUBVECTOR,(MVT::SimpleValueType)VT, Expand);<br></blockquote><blockquote type="cite">+ setOperationAction(ISD::CONCAT_VECTORS,(MVT::SimpleValueType)VT, Expand);<br></blockquote><br>What are these changes for?<br><br></div></blockquote><div><br></div><div>I don't need them anymore so I can remove it.</div><br><blockquote type="cite"><div><blockquote type="cite">+/// If there is no vector type that we want to widen to, returns MVT::Other<br></blockquote><blockquote type="cite">+/// When and were to widen is target dependent based on the cost of<br></blockquote><br>As above.<br><br><blockquote type="cite">+MVT X86TargetLowering::getWidenVectorType(MVT VT) {<br></blockquote><br>As I mentioned above, I think all this logic should be in the type<br>action, not here.</div></blockquote><div><br></div><div><br></div><div>I agree.</div><div><br></div><blockquote type="cite"><div><br><br><blockquote type="cite">+ /// If there is no vector type that we want to widen to, returns MVT::Other<br></blockquote><blockquote type="cite">+ /// When and were to widen is target dependent based on the cost of<br></blockquote><br>As above.<br><br>Ciao,<br><br>Duncan.<br></div></blockquote></div><br></div><div><br></div><div>Thanks for your comments!</div><div><br></div><div> -- Mon Ping</div></body></html>