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

[Michael Liao]

Done in r162780. Thanks for suggestion.

Yours
- Michael

On Tue, 2012-08-28 at 11:53 -0700, Evan Cheng wrote:
> Sorry, more nitpick:
> 
> +      Opnds.push_back(I->getOperand(0));
> +      Opnds.push_back(I->getOperand(1));
> +      // Re-evaluate the number of nodes to be traversed.
> +      e = Opnds.size();
> 
> Can't you replace it with?
> e += 2;
> 
> Evan
> 
> On Aug 27, 2012, at 3:38 PM, Michael Liao <michael.liao at intel.com> wrote:
> 
> > On Mon, 2012-08-27 at 14:39 -0700, Evan Cheng wrote:
> >> 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) {
> > 
> > As the loop body will append elements into Opnds, this will quit the
> > loop earlier than expectation. To avoid per-iteration Opnds.size(), I
> > need extra 'e' updating once this small vector is updated. Here is the
> > revised patch.
> > 
> > Yours
> > - Michael
> > 
> >> 
> >> 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
> >>> 
> >>> 
> >> 
> > 
> > <0001-add-X86-DAG-optimization-to-recognize-a-pttern-PTEST.patch>
>