[llvm-commits] InstCombine patch to improve combining of nested ShuffleVectorInst's

[Eli Friedman]

On Thu, Oct 20, 2011 at 1:35 PM, Guo, Xiaoyi <Xiaoyi.Guo at amd.com> wrote:
> Ping?

r142671.

-Eli

> -----Original Message-----
> From: Guo, Xiaoyi
> Sent: Tuesday, October 18, 2011 3:53 PM
> To: 'Eli Friedman'
> Cc: llvm-commits at cs.uiuc.edu
> Subject: RE: [llvm-commits] InstCombine patch to improve combining of nested ShuffleVectorInst's
>
> Hi Eli,
>
> Thank you for your careful review. I have made the changes according to your comments. I also had to make the getShuffleMask() interface change to return a SmallVector because otherwise I can't do the == compare with newMask.
>
> Attached please find the updated patch. If it looks okay, would you please help commit it?
>
> Thanks,
> Xiaoyi
>
> -----Original Message-----
> From: Eli Friedman [mailto:eli.friedman at gmail.com]
> Sent: Tuesday, October 18, 2011 2:04 PM
> To: Guo, Xiaoyi
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] InstCombine patch to improve combining of nested ShuffleVectorInst's
>
> On Mon, Oct 17, 2011 at 3:51 PM, Guo, Xiaoyi <Xiaoyi.Guo at amd.com> wrote:
>> Ping?
>>
>> -----Original Message-----
>> From: llvm-commits-bounces at cs.uiuc.edu
>> [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Eli Friedman
>> Sent: Tuesday, October 11, 2011 4:34 PM
>> To: Guo, Xiaoyi
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] InstCombine patch to improve combining of
>> nested ShuffleVectorInst's
>>
>> On Tue, Oct 11, 2011 at 4:16 PM, Guo, Xiaoyi <Xiaoyi.Guo at amd.com> wrote:
>>> From the comment in the original code I understood that we want to be very conservative when doing the shuffle transformations. So I also tried to be careful and only do the transformation if no new mask is created, so the net effect is just removing some instructions, which I think should be as safe as the original code.
>>
>> Oh, I missed that part.  Now I follow what you're doing. :)
>>
>> The concept is fine; I want to look over the mask transform logic a bit more closely before committing, though.
>
> Sorry about the delay.
>
> +  std::vector<int> LHSMask;
> +  std::vector<int> RHSMask;
> +  if (newLHS != LHS) {
> +    LHSMask = getShuffleMask(LHSShuffle);  }  if (RHSShuffle && newRHS
> + != RHS) {
> +    RHSMask = getShuffleMask(RHSShuffle);  }
>
> I would normally say this needs to be changed, but it looks like existing badness in the implementation of getShuffleMask.  Might be nice to fix getShuffleMask at some point.
>
> +  std::vector<int> newMask;
> +  bool isSplat = true;
> +  int SplatElt = -1;
>
> Please use a SmallVector.
>
> +  // If the result mask is equal to one of the original shuffle mask,
> + // do the replacement.
> +  if (isSplat || newMask == LHSMask || newMask == RHSMask || newMask ==
> + Mask) {
>
> Comment doesn't quite match the code.
>
> +    std::vector<Constant*> Elts;
> +    Type *Int32Ty = Type::getInt32Ty(SVI.getContext());
> +    for (unsigned i = 0, e = newMask.size(); i != e; ++i) {
>
> SmallVector
>
> +    // Check if this could still be a splat.
> +    if (eltMask >= 0) {
> +      if (SplatElt >=0 && SplatElt != eltMask)
>
> Whitespace.
>
> The for loop building up newMask could generally use a few more comments explaining what mapping each case is handling.
>
> Otherwise, it looks good.
>
> -Eli
>
>
>