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

[Guo, Xiaoyi]

Ping?

-----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