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

[Michael Zolotukhin]

Hi Karthik,

Thanks for the comments, please find my answers inline (there is a couple of them in the tests too).


REPOSITORY
  rL LLVM

================
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]);
----------------
karthikthecool wrote:
> 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.
> 
Yep, I'd prefer checking for broadcast before the loop.

Actually, checking for AllSameOpcode before the reordering loop now seems useless - anyway it's invalidated when we swap operands. So, what do you think we should do here? Check for broadcasts, reorder commutative operands, check for AllSameOperand, then revert to original version if all of these attempts failed?

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2308-2311
@@ -2220,1 +2307,6 @@
+      else {
+        for (int i = 0, e = E->Scalars.size(); i < e; ++i) {
+          LHSVL.push_back(cast<Instruction>(E->Scalars[i])->getOperand(0));
+          RHSVL.push_back(cast<Instruction>(E->Scalars[i])->getOperand(1));
+        }
       }
----------------
My understanding is that we only vectorize ShuffleVectors if it's AltInst, which means VL0 should always be a BinaryOperator. Am I wrong here? If I'm not, please add an assert and remove if-else here.

http://reviews.llvm.org/D6677

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