[llvm-commits] [llvm] r145487 - /llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

[Craig Topper]

Thanks for the comments. I forgot to "svn add" the test cases. I'll commit
it tonight if I have time. Also forgot to make it work for 256-bit FP. I'll
add the asserts and try to clean up the loops as well. I think there's an
80 column violation in there too.

On Wed, Nov 30, 2011 at 1:47 AM, Duncan Sands <baldrick at free.fr> wrote:

> Hi Craig,
>
> > Add instruction selection support for AVX2 horizontal add/sub
> instructions.
>
> thanks for doing this.  Please add a testcase.
>
> > --- llvm/trunk/lib/Target/X86/X86ISelLowering.cpp (original)
> > +++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Wed Nov 30 03:10:50
> 2011
> > @@ -14329,7 +14329,9 @@
> >       return false;
> >
> >     EVT VT = LHS.getValueType();
> > -  unsigned N = VT.getVectorNumElements();
> > +  unsigned NumElts = VT.getVectorNumElements();
> > +  unsigned NumLanes = VT.getSizeInBits()/128;
>
> Here you assume that the number of bits is always a multiple of 128.
>  While this
> is currently true (and probably always will be) how about adding an
> assertion
> checking this?
>
> > +  for (unsigned l = 0; l != NumLanes; ++l) {
> > +    unsigned LaneStart = l*NumLaneElts;
> > +    for (unsigned i = 0; i != NumLaneElts/2; ++i) {
>
> Here you assume that NumLaneElts is even, in particular that it is not 1.
>  Is it
> really impossible for it to be equal to 1?  If it is impossible, please
> add an
> assertion that checks that.  If it is possible, please handle it!
>
> > +      unsigned LIdx = LMask[i+LaneStart];
> > +      unsigned RIdx = RMask[i+LaneStart];
> > +
> > +      // Ignore any UNDEF components.
> > +      if (LIdx>= 2*NumElts || RIdx>= 2*NumElts ||
> > +          (!A.getNode()&&  (LIdx<  NumElts || RIdx<  NumElts)) ||
> > +          (!B.getNode()&&  (LIdx>= NumElts || RIdx>= NumElts)))
> > +        continue;
> > +
> > +      // Check that successive elements are being operated on.  If not,
> this is
> > +      // not a horizontal operation.
> > +      if (!(LIdx == 2*i + LaneStart&&  RIdx == 2*i + LaneStart + 1)&&
> > +          !(isCommutative&&  LIdx == 2*i + LaneStart + 1&&  RIdx == 2*i
> + LaneStart))
> > +        return false;
> > +    }
>
> This loop and the one below are essentially doing the same thing.  How
> about
> unifying them, but introducing an additional loop that loops over the
> values
> 0 and 1:
>   for (unsigned h = 0; h != 2; ++h)
>      for (unsigned i = 0; i != NumLaneElts/2; ++i) {
>        unsigned LIdx = LMask[i+LaneStart+h*(NumLaneElts/2)];
> and so on.
>
> Ciao, Duncan.
>
> PS: I didn't check the correctness of your multi-lane logic.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>



-- 
~Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111130/188c5517/attachment.html>