<div dir="ltr">Committed rev 218667, 218668, 218669.</div><div class="gmail_extra"><br><div class="gmail_quote">2014-09-29 21:26 GMT+04:00 Adam Nemet <span dir="ltr"><<a href="mailto:anemet@apple.com" target="_blank">anemet@apple.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span class=""><div>On Sep 29, 2014, at 7:22 AM, Robert Khasanov <<a href="mailto:rob.khasanov@gmail.com" target="_blank">rob.khasanov@gmail.com</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr">Hi Adam,<br><div class="gmail_extra"><br><div class="gmail_quote">2014-09-28 11:14 GMT+04:00 Adam Nemet <span dir="ltr"><<a href="mailto:anemet@apple.com" target="_blank">anemet@apple.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span><div>On Sep 26, 2014, at 8:04 AM, Robert Khasanov <<a href="mailto:rob.khasanov@gmail.com" target="_blank">rob.khasanov@gmail.com</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr">Hi Adam,<div><br></div><div>Thanks for review.</div><div>I agree with you comment about reusing <span style="font-family:arial,sans-serif;font-size:13px">getVectorMaskingNode</span><span style="font-family:arial,sans-serif;font-size:13px">. See new patches attached.</span></div></div></blockquote><div><br></div></span><div>Is 003 the correct version now?</div><span><br></span></div></div></blockquote><div><br></div><div>yes. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><span><div></div><blockquote type="cite"><div dir="ltr">However, I don't understand your comment about ISD::INSERT_SUBVECTOR. Could you please clarify your suggestion?</div></blockquote><div><br></div></span><div>You’re making this op legal for v8i1, so we will now generate things like:</div><div><br></div><div> (v8i1 (insert_subvector (v8i1 foo), (v4i1 bar), (iPTR 0)))</div><div><br></div><div>Do we have instructions to match these in the TD file?</div><span><font color="#888888"><br></font></span></div></blockquote><div><br></div><div>We generate things like (v8i1 (insert_subvector undef, (v4i1 src), (iPTR 0))).</div><div>For this we have following rules in TD (see 003 patch):</div><div><br></div><div><div>+ def : Pat<(v8i1 (insert_subvector undef, (v4i1 VK4:$src), (iPTR 0))),</div><div>+ (v8i1 (COPY_TO_REGCLASS VK4:$src, VK8))>;</div><div>+ def : Pat<(v8i1 (insert_subvector undef, (v2i1 VK2:$src), (iPTR 0))),</div><div>+ (v8i1 (COPY_TO_REGCLASS VK2:$src, VK8))>;</div></div></div></div></div></blockquote><div><br></div></span><div>OK, I missed this one.</div><div><br></div><div>You still owe me :) the sample DAG in the comment to: </div><div><br></div><div><blockquote type="cite"><div>+ SDValue Mask = Op.getOperand(3);</div><div>+ EVT BitcastVT = EVT::getVectorVT(*DAG.getContext(), MVT::i1,</div><div>+ Mask.getValueType().getSizeInBits());</div><span class=""><div> SDValue Cmp = DAG.getNode(IntrData->Opc0, dl, MaskVT,</div></span><div> Op.getOperand(1), Op.getOperand(2));</div><div>- SDValue Res = getVectorMaskingNode(Cmp, Op.getOperand(3),</div><div>- DAG.getTargetConstant(0, MaskVT), DAG);</div><div>+ SDValue CmpMask = getVectorMaskingNode(Cmp, Op.getOperand(3),</div><div>+ DAG.getTargetConstant(0, MaskVT), DAG);</div><div>+ SDValue Res = DAG.getNode(ISD::INSERT_SUBVECTOR, dl, BitcastVT,</div><div>+ DAG.getUNDEF(BitcastVT), CmpMask,</div><div>+ DAG.getIntPtrConstant(0));</div></blockquote><br></div><div>Other than that LGTM, thanks for splitting it up like this.</div><div><br></div><div>One more question more of as a follow-on discussion. Do we actually have to generate AND for masking compares? I understand that that is what they get canonicalized into but perhaps we can still generate vselect uniformly.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>Adam</div></font></span><div><div class="h5"><div><br></div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><div>Thanks,</div><div>Robert </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><span><font color="#888888"><div></div><div>Adam</div></font></span><div><div><br><blockquote type="cite"><div dir="ltr"><div>Thanks, </div><div>Robert </div></div><div class="gmail_extra"><br><div class="gmail_quote">2014-09-26 4:19 GMT+04:00 Adam Nemet <span dir="ltr"><<a href="mailto:anemet@apple.com" target="_blank">anemet@apple.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div>On Sep 25, 2014, at 12:43 AM, Robert Khasanov <<a href="mailto:rob.khasanov@gmail.com" target="_blank">rob.khasanov@gmail.com</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr"><div>The last patch is incorrect. See this version instead of previous one.</div><div>Sorry for that.</div></div><div class="gmail_extra"><br><div class="gmail_quote">2014-09-25 11:40 GMT+04:00 Robert Khasanov <span dir="ltr"><<a href="mailto:rob.khasanov@gmail.com" target="_blank">rob.khasanov@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi Elena, Adam,<div><br></div><div>In these patches I enable intrinsics for vpcmpeq{bwdq} instructions.</div><div>Since result of the instructions is mask, I enable new intrinsics type CMP_MASK and assume to enable CMP_MASK_CC in future for vpcmp{bwdq} instructions (due to additional CC argument).</div><div>Also I extended argument types when intrinsics generated through TableGen (IIT_V64) to support 64 packed data.</div><div>In last patch I enabled INSERT_SUBVECTOR for v8i1 to legalizer to support converting v2i1 and v4i1 to v8i1 and then bitcasting to i8.</div><div><br></div><div><span style="font-family:arial,sans-serif;font-size:13px">Please let me know if it looks good.</span><span><font color="#888888"><br></font></span></div></div></blockquote></div></div></blockquote><div><br></div></div><div>Hi Robert,</div><div><br></div><div><div>+ case CMP_MASK: {</div><div>+ EVT VT = Op.getOperand(1).getValueType();</div><div>+ EVT MaskVT = EVT::getVectorVT(*DAG.getContext(), MVT::i1,</div><div>+ VT.getVectorNumElements());</div><div>+ SDValue Cmp = DAG.getNode(IntrData->Opc0, dl, MaskVT,</div><div>+ Op.getOperand(1), Op.getOperand(2));</div><div>+ SDValue Res;</div><div>+ if (isAllOnes(Op.getOperand(3))) {</div><div>+ Res = Cmp;</div><div>+ } else {</div><div>+ Res = DAG.getNode(ISD::AND, dl, MaskVT, Cmp,</div><div>+ DAG.getNode(ISD::BITCAST, dl, MaskVT,</div><div>+ Op.getOperand(3)));</div><div>+ }</div><div>+ return DAG.getNode(ISD::BITCAST, dl, Op.getValueType(), Res);</div><div>+ }</div><div><br></div><div>It would be good to reuse (and extend if needed) the helper getVectorMaskingNode for this. That should capture what it takes from a expression to become a masked expression in one place (just like AVX512_masking in td). Obviously the BITCAST at the end does not belong there there but the information that masked compare is canonicalized to an AND rather than a VSELECT is useful.</div><div><br></div><div>As you see I also don’t check for the mask value to be AllOnes in getVectorMaskingNode. We can add that for now if you need but I think that should be an orthogonal DAGCombiner transformation to cover more cases.</div><div><br></div><div><div>+ setOperationAction(ISD::INSERT_SUBVECTOR, MVT::v8i1, Legal);</div><div><br></div><div>Can you just enable it like that? I understand that you need it to lower the intrinsics but you may need to add some more patterns to TD to recognize this in the general case.</div></div><div><br></div><div><div>- Res = DAG.getNode(ISD::AND, dl, MaskVT, Cmp,</div><div>- DAG.getNode(ISD::BITCAST, dl, MaskVT,</div><div>- Op.getOperand(3)));</div><div>+ RMask = DAG.getNode(ISD::AND, dl, MaskVT, Cmp,</div><div>+ DAG.getNode(ISD::EXTRACT_SUBVECTOR, dl, MaskVT,</div><div>+ DAG.getNode(ISD::BITCAST, dl, BitcastVT, Mask),</div><div>+ DAG.getIntPtrConstant(0)));</div><div> }</div><div>- return DAG.getNode(ISD::BITCAST, dl, Op.getValueType(), Res);</div><div>+ return DAG.getNode(ISD::BITCAST, dl, Op.getValueType(),</div><div>+ DAG.getNode(ISD::INSERT_SUBVECTOR, dl, BitcastVT,</div><div>+ DAG.getUNDEF(BitcastVT), RMask,</div><div>+ DAG.getIntPtrConstant(0)));</div><div><br></div><div>This could use a comment with an example DAG that we’re trying to create.</div></div><div><br></div><div>Thanks,</div><div>Adam</div></div><div><br></div><br><blockquote type="cite"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><span><font color="#888888"><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">Robert</span></div></font></span></div>
</blockquote></div><br></div>
</blockquote></div></div><br><div style="word-wrap:break-word"><div><blockquote type="cite"></blockquote></div><br></div>
<br></blockquote></div><br></div>
</blockquote></div></div></div><br><div style="word-wrap:break-word"><blockquote type="cite"></blockquote></div>
<br><div style="word-wrap:break-word"><blockquote type="cite"></blockquote></div>
<br><div style="word-wrap:break-word"><div><blockquote type="cite"></blockquote></div><br></div>
<br></blockquote></div><br></div></div>
</blockquote></div></div></div><br></div></blockquote></div><br></div>