[PATCH] [SLPVectorizer] Reorder operands of shufflevector if it can result in a vectorized code.

[Karthik Bhat]

Please find my comments inline. Thanks.


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1879-1880
@@ +1878,4 @@
+      (AllSameOpcodeRight || AllSameOpcodeLeft)) {
+    Left = OrigLeft;
+    Right = OrigRight;
+  }
----------------
mzolotukhin wrote:
> You might want to return from here. Otherwise, the broadcast/allSameOpcode could be spoiled by the following it code.
We cannot return from here as in case of allSameOpcode as well we still might want to reorder.
For instance in the example which you mentioned in previous comments-
e.g.
  c[0] = a[0] + b[0];
  c[1] = b[1] + a[1]; // a[1] and b[1] are swapped

all the left and right operands have same opcode i.e. load. So AllSameOpcodeRight and AllSameOpcodeLeft would be true but we still want to reorder those loads if they were commutative across instruction as in the example.
Yes if this was a broadcast we might not want to reorder and keep original order for which we have the check in the below code as -
  !(Left[j] == Left[j + 1] || Right[j] == Right[j + 1])
This implies that if this insturction which we are trying to reorder is part of a broadcast then do not reorder.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1900
@@ +1899,3 @@
+        if (isConsecutiveAccess(L, L1) &&
+            !(Left[j] == Left[j + 1] || Right[j] == Right[j + 1])) {
+          std::swap(Left[j + 1], Right[j + 1]);
----------------
mzolotukhin wrote:
> This check looks strange. Why do we bail out if e.g. left-operands are the same? If **all** left-elements are the same, we should have `AllSameOpcode`, otherwise I see no special value in keeping them together.
> 
> I guess that here you wanted to discard some cases, when we don't want to change anything not to spoil already good disposition. If that's the case, I think we need a better check to detect such cases (e.g. `Left[i]` and `Left[i+1]` are consecutive or something like this).
Hi Michael,
As mentioned in previous comments. AllSameOpcode just checks if all the opcodes of the instructions are same not necessarily the instructions itself. To check if this is a broadcast we need to compare the actual instructions. Hence this check was added to make sure that we do not reorder anything that is part of broadcast.
 
Now that i think about it i think we can check the LeftBroadcast and RightBroadcast before entering the loop and avoid this check inside the loop.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1903
@@ +1902,3 @@
+        }
+        // else unchanged
+      }
----------------
mzolotukhin wrote:
> It is possible that `Left[i]` and `Right[i+1]` are not consecutive, yet the swap is beneficial.
> 
> See the following example:
> ```
> a[0] + b[0]
> b[1] + c[1]
> c[2] + d[2]
> d[3] + e[3]
> ```
> 
> In this scenario, the current code won't swap anything, but it could be transformed to
> ```
> a[0] + b[0]
> c[1] + b[1]
> c[2] + d[2]
> e[3] + d[3]
> ```
> if we check `Right[i]` Vs `Left[i+1]` too.
> 
> It's an artificial example though, and maybe it's not worth a code to handle it (but I'm not sure if the original code we are trying to handle isn't artificial as well). It would be great btw if you share the motivation for this changes - have you seen something like this in real-world tests?
> 
I initally wanted to just handle this for shuffle vectors as I had found some hand written code in our codebase which had that pattern but extended it generaically for any commutative operands as per review comments from arnold. As you mentioned I'm not sure if it is worth to handle all combinations but will add this check anyway as it doesn't seem to be a lot of overhead.

http://reviews.llvm.org/D6677

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/