<div dir="ltr">Hi Chandler,<div><br></div><div>In addition to Adam's comment (Thanks Adam for answers), you can see thread <a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140929/237328.html">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140929/237328.html</a></div><div><br></div><div>If you feel strongly, I can split this patch into two: VSELECT simplification (with test) and changes in intrinsics lowering.<br></div><div><br></div><div>Robert</div></div><div class="gmail_extra"><br><div class="gmail_quote">2014-10-01 0:16 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 30, 2014, at 11:04 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">This patch doesn't really make sense to me...</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 30, 2014 at 10:26 AM, Robert Khasanov <span dir="ltr"><<a href="mailto:rob.khasanov@gmail.com" target="_blank">rob.khasanov@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="overflow:hidden">[x86] Simplify vector selection if condition value type matches vselect value type and true value is all ones or false value is all zeros.<br>
This transformation worked if selector is produced by SETCC, however SETCC is needed only if we consider to swap operands. So I replaced SETCC check for this case.<br></div></blockquote><div><br></div><div>What is your actual goal here?</div></div></div></div></blockquote><div><br></div></span><div>The goal was to separate the lowering of the CMPM intrinsics and a related canonicalization/simplification.   This is the simplification:</div><div><br></div><div>(vselect VK8:$mask, (PCMPEQM %a, %b), (v8i1 (BUILD_VECTOR 0, 0, 0, 0, 0, 0, 0, 0)))</div><div><br></div><div>to</div><div><br></div><div>(and VK8:$mask, (PCMPEQM %a, %b))</div><div><br></div><div>Right now in the intrinsic lowering code we special-case CMPM and lower it to an AND expression rather than the generic VSELECT.  My comment on a previous patch was to avoid special-casing like this and letting the DAGCombiner do its job.  This is what the patch is trying to do.</div><span class=""><div><br></div><br><blockquote type="cite"><div dir="ltr"><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 style="overflow:hidden">
Also I removed special case for cmp instructions in getVectorMaskingNode. Now cmp intrinsics lower as other intrinsics through VSELECT, and then VSELECT tranforms to AND in PerformSELECTCombine.<br></div></blockquote><div><br></div><div>This should be a separate patch.</div></div></div></div></blockquote><div><blockquote type="cite"><br></blockquote></div><blockquote type="cite"><div dir="ltr"><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 style="overflow:hidden">
New tests are not added, existed intrinsics tests on vpcmpeq{bwdq} cover these cases.</div></blockquote></div><br>I don't get it.</div></div></blockquote></span><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><br></div><div class="gmail_extra">Does this patch have no functional impact? If so, say so, and justify both why it should have no functional impact and why the refactoring is desirable? The change looks like it *should* have functional impact…</div></div></blockquote><div><br></div><div>It shouldn’t have functional impact, it just stages the lowering differently:</div><div><br></div><div>intrinsic -> AND   // all done by intrinsic lowering</div><div><br></div><div>intrinsic -> VSELECT -> AND  // first is done by lowering and the second by the DAGCombiner</div><div><br></div><div>And this is why the old test which ensures that at the end we do generate the cmpm can cover this patch.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>Adam</div></font></span><span class=""><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">Also, you did see the recent churn on VSELECT? I'm planning to implement the new plan I outlined in my commit log. It would be great for you to review those patches and the plan I outlined and comment on it (I asked for comments).</div></div>
</blockquote></span></div><br></div></blockquote></div><br></div>