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

[Eli Friedman]

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