[llvm-commits] [PATCH 2/2] FIx PR12312

[Evan Cheng]

Sorry, just got back.

One nitpick:

for (unsigned Slot = 0; Slot < Opnds.size(); ++Slot) {

You want something like this to avoid calling Opnds.size() in every iteration.
for (unsigned Slot = 0, e = Opnds.size(); Slot < e; ++Slot) {

Otherwise LGTM.

Evan

On Aug 18, 2012, at 9:15 PM, Michael Liao <michael.liao at intel.com> wrote:

> ping again. - Michael
> 
> On Tue, 2012-08-14 at 23:49 -0700, Michael Liao wrote:
>> Here is the revised patch putting 128-bit vector checking earlier.
>> 
>> Yours
>> - Michael
>> 
>> On Tue, 2012-08-14 at 13:53 -0700, Evan Cheng wrote:
>>> +  if (VT.is128BitVector())
>>> +    VecIn = DAG.getNode(ISD::BITCAST, DL, MVT::v2i64, VecIn);
>>> +  else // FIXME: only 128-bit vector is supported so far.
>>> +    return SDValue();
>>> 
>>> It seems like this check can happen earlier to avoid doing a bunch of work for no good reason.
>>> 
>>> Evan
>>> 
>>> On Aug 13, 2012, at 1:25 PM, Michael Liao <michael.liao at intel.com> wrote:
>>> 
>>>> Here is another revision of the 2nd part. The new change fixes an
>>>> potential issue of stale iterator being used. The new loop BFS
>>>> traversing OR'd tree is now over index instead of iterator as we may add
>>>> elements in that loop.
>>>> 
>>>> Yours
>>>> - Michael
>>>> 
>>>> On Sun, 2012-08-12 at 14:28 -0700, Michael Liao wrote:
>>>>> On Fri, 2012-08-10 at 14:27 -0700, Evan Cheng wrote:
>>>>>> +static SDValue FlaggedOrCombine(SDValue Or, X86::CondCode &CC, SelectionDAG &DAG,
>>>>>> +                                const X86Subtarget *Subtarget) {
>>>>>> 
>>>>>> 
>>>>>> The current naming convention calls for lower cased first letter.
>>>>> 
>>>>> The revised patch is attached. I checked the function naming in the 1st
>>>>> part of patch as well.
>>>>> 
>>>>>> 
>>>>>> 
>>>>>> +  if (VecIn != DAG.getVectorShuffle(VT, DL, VecIn, VecIn, &Mask[0]))
>>>>>> +    return SDValue();
>>>>>> 
>>>>>> What is this trying to do? Is it potentially creating a vectorshuffle node and then throwing it away? Please don't do that.
>>>>>> 
>>>>> 
>>>>> It is checking whether the all vector elements are used in that OR'd
>>>>> tree. I revised the code to use a bit mask for this purpose and simplify
>>>>> both this checking and other parts.
>>>>> 
>>>>> Please review the revised patch attached.
>>>>> 
>>>>> Yours
>>>>> - Michael
>>>>> 
>>>>>> Evan
>>>>>> 
>>>>>> On Aug 1, 2012, at 12:47 PM, Michael Liao <michael.liao at intel.com> wrote:
>>>>>> 
>>>>>>> Hi
>>>>>>> 
>>>>>>> This patch is to fix http://llvm.org/bugs/show_bug.cgi?id=12312, where a
>>>>>>> special use of i128 needs efficient code generation with PTEST from
>>>>>>> SSE4.1.
>>>>>>> 
>>>>>>> To fix this issue, 2 patches are developed. The second part is to
>>>>>>> recognize that special use, casting from <4 x i32> to i128 followed by a
>>>>>>> zero (or non-zero) checking. That sequence is lowered into OR
>>>>>>> (X86ISD::OR at the root) tree with all leaves extracted from that vector
>>>>>>> because i128 is mapped onto GPR pair. The attached patch adds a
>>>>>>> X86-specific DAG optimization to recognize this OR'd tree and convert it
>>>>>>> a PTEST if only the flag value is used from that X86ISD::OR root node.
>>>>>>> 
>>>>>>> Yours
>>>>>>> - Michael
>>>>>>> <0002-add-X86-DAG-optimization-to-recognize-a-pttern-PTEST.patch>_______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> 
>>>> <0001-add-X86-DAG-optimization-to-recognize-a-pttern-PTEST.patch>
>>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
>