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

[Michael Zolotukhin]

Hi Karthik,

Thanks for the updated patch, please see my comments inline.

As for the AllSameOpcode changes - your fix looks right anyway, and I think it might go independently of this patch.


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1879-1880
@@ +1878,4 @@
+      (AllSameOpcodeRight || AllSameOpcodeLeft)) {
+    Left = OrigLeft;
+    Right = OrigRight;
+  }
----------------
You might want to return from here. Otherwise, the broadcast/allSameOpcode could be spoiled by the following it code.

================
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]);
----------------
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).

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1903
@@ +1902,3 @@
+        }
+        // else unchanged
+      }
----------------
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?


================
Comment at: test/Transforms/SLPVectorizer/X86/addsub.ll:254
@@ +253,3 @@
+;  c[0] = (a[0]+b[0])-d[0];
+;  c[1] = d[1]+(a[1]+b[1]) //swapped d[1] and (a[1]+b[1]) 
+
----------------
Missed `;` at the end of the line :)

================
Comment at: test/Transforms/SLPVectorizer/X86/addsub.ll:280-283
@@ +279,6 @@
+
+; CHECK-LABEL: @no_vec_shuff_reorder
+; CHECK-NOT: fadd <4 x float>
+; CHECK-NOT: fsub <4 x float>
+; CHECK-NOT: shufflevector
+define void @no_vec_shuff_reorder() #0 {
----------------
What about C-code for this test?

http://reviews.llvm.org/D6677

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