<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Hi Michael, </div><div><br></div><div>Sorry for the delay. LGTM. </div><div><br></div><div>Thanks,</div><div>Nadav</div><br><div><div>On Mar 1, 2013, at 9:02 AM, Michael Liao <<a href="mailto:michael.liao@intel.com">michael.liao@intel.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;">ping<br><br>- michael<br><br>On Wed, 2013-02-27 at 11:56 -0800, Michael Liao wrote:<br><blockquote type="cite">Hi Nadav<br><br>It's possible but that will duplicate the logic handling vector type for<br>each target. There are several options.<br><br>+ current approach: rename the original getShiftAmountTy() to<br>getScalarShiftAmountTy() and enhance the logic in getShiftAmountTy() to<br>handle vector type. Out-of-tree targets need to change their name only.<br><br>+ keep the name of getShiftAmountTy() and enhance its logic to handle<br>vector type. All targets will duplicate that logic. Out-of-tree targets<br>still need that change duplicating the logic.<br><br>+ keep the name of getShiftAmountTy() and add another new method to<br>handle vector type. We have to fix all getShiftAmountTy() occurrences in<br>TICG plus all targets if they use them for vector shift.<br><br>The current approach sounds to me a cleaner way to put new constraints<br>on vector shift ops. Comments?<br><br>BTW, they will break if out-of-tree targets rely on scalar shift amount<br>on vector shift.<br><br>Yours<br>- Michael<br><br><br>On Wed, 2013-02-27 at 11:32 -0800, Nadav Rotem wrote:<br><blockquote type="cite">The change looks correct. I am a little worried about out-of-tree<br>targets. Would it be possible to keep the original name of<br>GetShiftAmountTy ?<br><br><br>Thanks,<br>Nadav<br><br>On Feb 27, 2013, at 10:59 AM, Michael Liao <<a href="mailto:michael.liao@intel.com">michael.liao@intel.com</a>><br>wrote:<br><br><blockquote type="cite">Hi Nadav<br><br>Continue the approach we discussed, after fixing more places in<br>TICG,<br>PR10475 is fixed. Please review the attached patch.<br><br>Yours<br>- Michael<br><br>On Fri, 2013-02-22 at 13:36 -0800, Michael Liao wrote:<br><blockquote type="cite">Hi Nadav<br><br>On Fri, 2013-02-22 at 13:30 -0800, Nadav Rotem wrote:<br><blockquote type="cite">Hi Michael,<span class="Apple-converted-space"> </span><br><br>I prefer not to work around problems when we can fix them. You<br>can fix TLI.getShiftAmountTy to return vectors. It is obvious to<br>me that the shift amount should match the shifted data type.<br>This is the way it is in the LLVM IR, and this is the way it is<br>in most parts of SelectionDAG. We need to fix this. You don't<br>have to do it yourself, but please don't add more workarounds.<br></blockquote><br>That's my first attempt to solve this issue as I have the same<br>feeling<br>ISD::SHL should to be consistent with LLVM IR. However, the change<br>needs<br>more effort as EVT instead of MVT will be returned from<br>TLI.getShiftAmountTy() and lots of places in TICG assumes that<br>being an<br>MVT.<br><br>Technically, allowing a scalar shift amount in vector shift is a<br>legal<br>type in many targets, such as ARM NEON. Just ask thoughts where we<br>consider relax ISD::SHL. Anyway, I have no preference on either<br>approach.<br><br>Yours<br>- Michael<br><blockquote type="cite"><br>Thanks,<br>Nadav<br><br>On Feb 21, 2013, at 9:36 AM, Michael Liao<br><<a href="mailto:michael.liao@intel.com">michael.liao@intel.com</a>> wrote:<br><br><blockquote type="cite">Hi Nadav<br><br>On Thu, 2013-02-21 at 09:12 -0800, Nadav Rotem wrote:<br><blockquote type="cite">I think that we should make sure that the shifted value and<br>the amount are either both vectors or both scalars. I<br>looked at it in the past and wanted to add assertions, but<br>the Cell backend relied on this behavior. But now that the<br>Cell backend is gone we can add the assertions and catch all<br>of the places that violate this rule.<span class="Apple-converted-space"> </span><br></blockquote><br>That will break all shifts generated within TICG. All code<br>there call<br>TLI.getShiftAmountTy() to create the shift amount operand. But<br>all<br>shifts generated in TICG fall into vector-scalar shift form.<br>There's<br>another issue by forcing that. It will also break lots of<br>assertions<br>assuming ShiftAmountTy() to be MVT instead of EVT. With DAG<br>preparation,<br>the vector shift amount may have to an EVT (illegal type<br>mostly.)<br><br>Allowing shift-amount to be scalar or the same vector type if<br>the 1st<br>operand is an integer vector may has the minor impact for the<br>existing<br>code comparing ensuring them to be either both vectors or both<br>scalars.<br>What's your thoughts?<br><br>Yours<br>- Michael<br><br><blockquote type="cite"><br>Thanks,<br>Nadav<span class="Apple-converted-space"> </span><br><br>On Feb 20, 2013, at 11:52 PM, Michael Liao<br><<a href="mailto:michael.liao@intel.com">michael.liao@intel.com</a>> wrote:<br><br><blockquote type="cite">Hi All,<br><br>The root cause of PR10475 is the X86 vector shift lowering<br>logic assumes<br>the shift amount operand to be a vector. But all shift<br>nodes generated<br>by generic DAG combination/optimization, e.g. (Z-X) == X<br>--> Z == X<<1,<br>have scalar shift amount.<br><br>The documentation on ISD::SHL doesn't explicitly state<br>that there's no<br>such restriction on scalar or vector. (You could guess<br>that since it<br>mentioned it should be of type TLI.getShiftAmountTy(),<br>which always<br>return scalar type on all targets so far.) This may cause<br>misunderstanding as 'shl' in LLVM IR does have the<br>restriction as "Both<br>arguments to the ‘shl‘ instruction must be the same<br>integer or vector of<br>integer type." I'd like to revise the document if we think<br>it's<br>necessary.<br><br>BTW, another thought is that if shift-amount is allowed to<br>be a scalar,<br>we may add a generic DAG combination to detect cases where<br>a vector<br>shift-amount could be treated as a scalar. This could save<br>all targets<br>with similar vector-scalar-shift ops.<br><br>Yours<br>- Michael<br><0001-Fix-PR10475.patch>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote><br></blockquote><br><br></blockquote><br></blockquote><br></blockquote><br><0001-Fix-PR10475.patch></blockquote></blockquote></blockquote></div></blockquote></div><br></body></html>