[PATCH] Teach the DAGCombiner how to fold a OR of two shufflevector into a single shufflevector node

[Hal Finkel]

----- Original Message -----
> From: "Andrea Di Biagio" <andrea.dibiagio at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Nadav Rotem" <nrotem at apple.com>, "llvm-commits at cs.uiuc.edu for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Wednesday, March 5, 2014 1:30:28 PM
> Subject: Re: [PATCH] Teach the DAGCombiner how to fold a OR of two shufflevector into a single shufflevector node
> 
> Hi Hal and Nadav,
> 
> thanks for the very useful feedback.
> 
> I personally don't have a strong opinion on this.
> The patch can be changed to use 'isShuffleMaskLegal' , something
> like:
>       if (CanFold && isShuffleMaskLegal(Mask, VT))
>           /* do the transformation */;

Yes, please do this.

> 
> If instead we decide to move this combine on the x86 backend,

I'd prefer you not do this, the transformation is also useful on PowerPC.

> then my
> question is:
> Does this have to be AVX/AVX2 only?
> I think this transformation might be beneficial for non-AVX targets
> as
> well; we can still call ' isShuffleMaskLegal' to see if the new
> shuffle would be legal.
> Ideally that method should verify that the subtarget supports the new
> shuffle instruction. Although I understand that the lack of coverage
> of ''isShuffleMaskLegal' is a concern..

Yep, and this will increase the coverage, which will be a good thing. I also have some patches that I'll send for review soon that use this interface, and so I'm fairly certain that things here will improve.

 -Hal

> 
> Andrea
> 
> On Wed, Mar 5, 2014 at 6:26 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> > ----- Original Message -----
> >> From: "Nadav Rotem" <nrotem at apple.com>
> >> To: "Hal Finkel" <hfinkel at anl.gov>
> >> Cc: "llvm-commits at cs.uiuc.edu for LLVM"
> >> <llvm-commits at cs.uiuc.edu>, "Andrea Di Biagio"
> >> <andrea.dibiagio at gmail.com>
> >> Sent: Wednesday, March 5, 2014 11:59:27 AM
> >> Subject: Re: [PATCH] Teach the DAGCombiner how to fold a OR of two
> >> shufflevector into a single shufflevector node
> >>
> >>
> >>
> >>
> >> On Mar 5, 2014, at 9:55 AM, Hal Finkel < hfinkel at anl.gov > wrote:
> >>
> >>
> >>
> >> ----- Original Message -----
> >>
> >>
> >> From: "Nadav Rotem" < nrotem at apple.com >
> >> To: "Andrea Di Biagio" < andrea.dibiagio at gmail.com >
> >> Cc: " llvm-commits at cs.uiuc.edu for LLVM" <
> >> llvm-commits at cs.uiuc.edu >
> >> Sent: Wednesday, March 5, 2014 11:47:53 AM
> >> Subject: Re: [PATCH] Teach the DAGCombiner how to fold a OR of two
> >> shufflevector into a single shufflevector node
> >>
> >> Hi Andrea,
> >>
> >> Thanks for working on this. Lowering shuffles is really difficult
> >> and we have >1000 LOC in the x86 backends for lowering shuffles.
> >> It
> >> is really difficult to predict which shuffles are supported and
> >> which patterns result in slow code. Until now LLVM was *not* to
> >> introducing new shuffle patterns. The idea was that the users that
> >> generate the shuffle patterns should know if the patterns are
> >> supported by the target. This makes sense for users that write
> >> their
> >> own math libraries using the clang vector syntax. Your patch
> >> introduces new shuffle patterns that some backends may not know
> >> how
> >> to lower. I think that the code should be okay for AVX and AVX2
> >> because you are not introducing cross-lane shuffles. So, I suggest
> >> that you move this patch into the X86 backend and protect it with
> >> the hasAVX flags. Does that make sense?
> >>
> >> Is this not what TLI->isShuffleMaskLegal is for? We avoid
> >> introducing
> >> new shuffle patterns at the IR level, especially during
> >> canonicalization, because we don't want to use target information
> >> at
> >> that stage. In the SDAG, I think that the situation is different.
> >> There we should use TLI->isShuffleMaskLegal (at the other
> >> associated
> >> legality checks) to control these kinds of optimizations.
> >> Opinions?
> >>
> >>
> >>
> >> This is also a good solution. The advantage is that multiple
> >> targets
> >> would be able to benefit from Andrea's new optimization. The
> >> disadvantage is that the coverage of isShuffleMaskLegal is
> >> minimal.
> >
> > Okay; I'd prefer to do that, and improve isShuffleMaskLegal and
> > related interfaces as necessary.
> >
> >  -Hal
> >
> >>
> >>
> >>
> >>
> >> -Hal
> >>
> >>
> >>
> >>
> >> Thanks,
> >> Nadav
> >>
> >>
> >>
> >> On Feb 26, 2014, at 7:07 AM, Andrea Di Biagio
> >> < andrea.dibiagio at gmail.com > wrote:
> >>
> >>
> >>
> >> <patch-dagcombine.diff>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>
> >>
> >> --
> >> Hal Finkel
> >> Assistant Computational Scientist
> >> Leadership Computing Facility
> >> Argonne National Laboratory
> >>
> >
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory