<div dir="ltr">Thanks!<div>And I'd rather get INSERT_SUBVECTOR to work right (what I meant was that it's probably broken in general :-) )- I think it's more natural here than explicitly zeroing out the high lanes. If I see that that doesn't work out, I'll make the change you suggest.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 27, 2016 at 9:39 AM, Wei Mi <span dir="ltr"><<a href="mailto:wmi@google.com" target="_blank">wmi@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Fri, May 27, 2016 at 9:20 AM, Michael Kuperstein <<a href="mailto:mkuper@google.com">mkuper@google.com</a>> wrote:<br>
> mkuper added a comment.<br>
><br>
> I agree the spill code is complete nonsense, but I don't think it should block this patch, for two reasons:<br>
><br>
> 1. We seem to have a real problem with inserts into oversize vectors (e.g. <16 x i32> on SSE2), but I'm not entirely sure it's a high priority. I'm not sure how much of those the vectorizer actually generates - although, for reductions, that may be more common, I haven't checked.<br>
><br>
<br>
</span>I don't think it is a general problem. No such stack saves/restores<br>
are generated if we turn off detectSADPattern for sad16_i8. I think<br>
the problem is because we use INSERT_SUBVECTOR. I tried the following<br>
code then the stack saves/restores were gone.<br>
<br>
  if (VT.getSizeInBits() > ResVT.getSizeInBits()) {<br>
<br>
    // Update part of elements of the reduction vector. This is done by first<br>
    // extracting a sub-vector from it, updating this sub-vector, and inserting<br>
    // it back.<br>
    SDValue SubPhi = DAG.getNode(ISD::EXTRACT_SUBVECTOR, DL, ResVT, Phi,<br>
                                 DAG.getIntPtrConstant(0, DL));<br>
    SDValue Res = DAG.getNode(ISD::ADD, DL, ResVT, Sad, SubPhi);<br>
    SmallVector<SDValue, 16> Ops(VT.getSizeInBits() /<br>
ResVT.getSizeInBits(), DAG.getConstant(0, DL, ResVT));<br>
    Ops[0] = Res;<br>
    return DAG.getNode(ISD::CONCAT_VECTORS, DL, VT, Ops);<br>
<span class="">  }<br>
<br>
> 2. More importantly, even with the spills, it still looks like a large net improvement. E.g. the SSE2 code for the sad16_i8 loop before this patch is:<br>
</span>> ...<br>
<span class="">> Are you ok with me committing as is, and filing a PR on the insert_subvector issue?<br>
><br>
<br>
</span>I think it is fine. Thanks.<br>
</blockquote></div><br></div>