[PATCH] [x86] Simplify vector selection

[Robert Khasanov]

Hi Chandler,

In addition to Adam's comment (Thanks Adam for answers), you can see thread
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140929/237328.html

If you feel strongly, I can split this patch into two: VSELECT
simplification (with test) and changes in intrinsics lowering.

Robert

2014-10-01 0:16 GMT+04:00 Adam Nemet <anemet at apple.com>:

>
> On Sep 30, 2014, at 11:04 AM, Chandler Carruth <chandlerc at google.com>
> wrote:
>
> This patch doesn't really make sense to me...
>
> On Tue, Sep 30, 2014 at 10:26 AM, Robert Khasanov <rob.khasanov at gmail.com>
> wrote:
>
>> [x86] Simplify vector selection if condition value type matches vselect
>> value type and true value is all ones or false value is all zeros.
>> 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.
>>
>
> What is your actual goal here?
>
>
> The goal was to separate the lowering of the CMPM intrinsics and a related
> canonicalization/simplification.   This is the simplification:
>
> (vselect VK8:$mask, (PCMPEQM %a, %b), (v8i1 (BUILD_VECTOR 0, 0, 0, 0, 0,
> 0, 0, 0)))
>
> to
>
> (and VK8:$mask, (PCMPEQM %a, %b))
>
> 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.
>
>
> 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.
>>
>
> This should be a separate patch.
>
>
> New tests are not added, existed intrinsics tests on vpcmpeq{bwdq} cover
>> these cases.
>>
>
> I don't get it.
>
>
> 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…
>
>
> It shouldn’t have functional impact, it just stages the lowering
> differently:
>
> intrinsic -> AND   // all done by intrinsic lowering
>
> intrinsic -> VSELECT -> AND  // first is done by lowering and the second
> by the DAGCombiner
>
> And this is why the old test which ensures that at the end we do generate
> the cmpm can cover this patch.
>
> Adam
>
> 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).
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141001/5c60193b/attachment.html>