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

[Andrea Di Biagio]

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 */;

If instead we decide to move this combine on the x86 backend, 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..

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